-
Notifications
You must be signed in to change notification settings - Fork 148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor import hook API to subsume all platform-dependent behavior #445
Conversation
This lets us move all path resolution into the import hook, making it independent of Node's `fs` module.
This does sacrifice the ability to use Windows-style paths, but it's a convenience function that does "magic" stuff to its argument anyways, so I don't see it as much of a loss.
No worries and thanks for the PR @valadaptive! This change looks good overall, will send a review shortly. Great to hear about the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed the review feedback.
Here are the perf numbers for the Uint8Array change:
Perf tables
With Buffer
:
fromBuffer | toBuffer | isValid | (ops/sec) |
---|---|---|---|
33,317,478 | 10,631,527 | 180,366,905 | ArrayInt.avsc |
3,468,574 | 2,534,513 | 128,928,596 | ArrayString.avsc |
7,582,504 | 8,709,989 | 197,814,804 | Bytes.avsc |
689,280 | 679,409 | 2,674,241 | Cake.avsc |
2,592,217 | 2,206,942 | 38,822,853 | Coupon.avsc |
57,056,424 | 12,550,699 | 113,024,376 | Double.avsc |
61,574,774 | 11,820,736 | 60,649,981 | Enum.avsc |
61,538,321 | 13,354,808 | 114,390,373 | Float.avsc |
2,589,185 | 2,428,255 | 5,927,018 | HistoryItem.avsc |
3,151,044 | 3,205,620 | 53,984,621 | Human.avsc |
77,905,322 | 13,794,184 | 112,848,911 | Int.avsc |
34,027,942 | 12,880,509 | 104,609,630 | Long.avsc |
293,783 | 378,512 | 939,211 | PciEvent.avsc |
27,126,506 | 8,379,302 | 30,641,460 | Person.avsc |
9,307,901 | 5,816,258 | 57,643,068 | ProtobufTest.avsc |
12,765,398 | 8,389,290 | 102,736,271 | String.avsc |
195,432 | 113,200 | 1,012,104 | Tile.avsc |
18,685,374 | 7,854,798 | 35,397,755 | Union.avsc |
1,480,839 | 1,066,944 | 4,599,581 | User.avsc |
With Uint8Array
+ Buffer
functions for string encoding / decoding:
fromBuffer | toBuffer | isValid | (ops/sec) |
---|---|---|---|
26,724,848 | 17,414,862 | 173,377,550 | ArrayInt.avsc |
3,818,504 | 2,697,317 | 90,991,170 | ArrayString.avsc |
21,207,217 | 9,736,062 | 166,809,986 | Bytes.avsc |
843,434 | 642,307 | 2,612,795 | Cake.avsc |
3,314,544 | 2,131,250 | 37,131,186 | Coupon.avsc |
79,127,110 | 25,606,001 | 233,078,420 | Double.avsc |
79,373,943 | 21,588,047 | 79,517,556 | Enum.avsc |
106,002,679 | 26,058,862 | 233,474,594 | Float.avsc |
3,487,554 | 2,564,875 | 6,504,066 | HistoryItem.avsc |
5,214,008 | 3,056,836 | 49,537,779 | Human.avsc |
98,066,155 | 25,794,709 | 221,886,611 | Int.avsc |
37,888,878 | 21,836,901 | 193,645,566 | Long.avsc |
387,944 | 325,432 | 919,929 | PciEvent.avsc |
28,923,151 | 14,010,220 | 31,192,161 | Person.avsc |
10,576,640 | 7,687,052 | 83,377,019 | ProtobufTest.avsc |
15,767,017 | 11,890,621 | 210,533,610 | String.avsc |
209,431 | 117,867 | 956,955 | Tile.avsc |
21,418,426 | 11,937,821 | 36,588,398 | Union.avsc |
1,545,082 | 1,177,994 | 4,776,210 | User.avsc |
With Uint8Array
+ TextEncoder
and TextDecoder
:
fromBuffer | toBuffer | isValid | (ops/sec) |
---|---|---|---|
30,183,380 | 17,957,895 | 172,694,579 | ArrayInt.avsc |
3,125,114 | 2,268,559 | 83,375,656 | ArrayString.avsc |
23,428,938 | 8,569,906 | 164,245,978 | Bytes.avsc |
825,634 | 716,936 | 2,630,647 | Cake.avsc |
3,522,090 | 1,849,812 | 45,443,725 | Coupon.avsc |
79,602,728 | 24,994,036 | 231,514,849 | Double.avsc |
77,108,732 | 21,662,621 | 95,794,477 | Enum.avsc |
106,725,988 | 26,097,755 | 230,470,649 | Float.avsc |
2,722,699 | 2,289,792 | 6,548,083 | HistoryItem.avsc |
5,321,199 | 2,920,507 | 70,267,637 | Human.avsc |
99,272,048 | 25,639,119 | 225,582,362 | Int.avsc |
37,210,513 | 21,793,239 | 195,846,122 | Long.avsc |
507,922 | 495,641 | 1,422,177 | PciEvent.avsc |
26,569,325 | 14,217,492 | 36,755,572 | Person.avsc |
10,310,916 | 9,077,304 | 91,752,339 | ProtobufTest.avsc |
14,347,258 | 12,477,982 | 210,278,354 | String.avsc |
232,691 | 119,787 | 944,537 | Tile.avsc |
18,955,988 | 11,204,323 | 45,527,142 | Union.avsc |
1,489,976 | 1,047,133 | 4,866,507 | User.avsc |
Not sure what the ArrayInt
result is about (in previous tests, they've been about equivalent) but everything else seems way faster.
Thanks @valadaptive. The numbers above look great. Did you mean to push changes to this branch? |
Yup, my bad! Changes have been properly pushed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the slow review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating. Looks good - just two minor changes and this should be ready to merge.
6cd780c
to
7cdd7a6
Compare
I've made those last few changes and also stopped exporting existsSync and readFileSync now that that functionality's been moved into files.js. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @valadaptive!
First off, apologies for leaving you hanging for a year! Some stuff came up and this fell off my radar for a while, and this seemed like it was going to be more fiddly and difficult than it ended up being.
This import hook API isolates all platform-specific path resolution stuff within the import hook itself. The hook now takes the path of the imported file and the path of the file that imported it, and calls the callback with the file contents, and the resolved path of the imported file. That second part is what stumped me on my last attempt.
I also started work last year on a "de-Buffer-ification" branch, but ran into some performance cliffs. I finally figured those out (except for TextEncoder/TextDecoder, which is still slower in Node sadly), and the code is now on GitHub. It's actually faster on most benchmarks than the
Buffer
version. It's still a proof-of-concept and will need to be cleaned up. If you're interested in merging it before the major version bump, maybe removeservices
first, so I don't have to port that over to useUint8Array
.