-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
use byte array instead of string for code fetch #1307
Conversation
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.
Nice work - LGTM. I'll wait for Kitson to review before merging.
let low = (byte & 0x0F) as usize; | ||
let high = (byte >> 4) as usize; | ||
out.push(DIGIT_TO_CHAR[high]); | ||
out.push(DIGIT_TO_CHAR[low]); |
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'm not sure this is better?
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.
It is worse in terms of readability. It is a tradeoff.
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.
What's the tradeoff? Is this faster?
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.
Yes, definitely faster. Formatter involves very complex execution path.
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'm skeptical - certainly this is faster - but I doubt it's 2X faster. Anyway it doesn't matter. This is readable enough.
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 bet 10x faster. The default impl of ToString
trait is based on formatter. It is quite slow. That is why many types has its own version of specialized impl. This was a big motivation of specilization feature.
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 find a benchmark data for concatenating strings https://github.com/hoodie/concatenation_benchmarks-rs.
Format is about 3x slower than join.
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.
wow, interesting...
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 accidentally find that "Hex encoding" is something very suitable for SIMD, it is used as an example in standard document. My version is the hex_encode_fallback
.
I would leave this optimization for future work.
source_map: string; // Non-empty only if cached. | ||
source_code: [ubyte]; | ||
output_code: [ubyte]; // Non-empty only if cached. | ||
source_map: [ubyte]; // Non-empty only if cached. |
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.
👍
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.
Just one comment... 👍
let low = (byte & 0x0F) as usize; | ||
let high = (byte >> 4) as usize; | ||
out.push(DIGIT_TO_CHAR[high]); | ||
out.push(DIGIT_TO_CHAR[low]); |
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.
wow, interesting...
js/os.ts
Outdated
sourceCode: codeFetchRes.sourceCode() || undefined, | ||
outputCode: codeFetchRes.outputCode() || undefined, | ||
sourceMap: codeFetchRes.sourceMap() || undefined | ||
sourceCode: sourceCode && new TextDecoder().decode(sourceCode), |
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.
one decoder up above likely would be more memory efficient... (slightly) instead of newing up a new decoder each property
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.
Good idea!
faf1e87
to
af21f02
Compare
af21f02
to
9d526eb
Compare
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.
LGTM : )
This reverts commit e976b3e. There is nothing technically wrong with this commit, but it's adding complexity to a big refactor (native ES modules denoland#975). Since it's not necessary and simply a philosophical preference, I will revert for now and try to bring it back later.
This reverts commit e976b3e. There is nothing technically wrong with this commit, but it's adding complexity to a big refactor (native ES modules denoland#975). Since it's not necessary and simply a philosophical preference, I will revert for now and try to bring it back later.
This reverts commit e976b3e. There is nothing technically wrong with this commit, but it's adding complexity to a big refactor (native ES modules denoland#975). Since it's not necessary and simply a philosophical preference, I will revert for now and try to bring it back later.
Fix #1113
cc @kitsonk