-
Couldn't load subscription status.
- Fork 3.9k
ARROW-4523,ARROW-4524: [JS] Add row proxy generation benchmark, improve performance #3601
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
Conversation
new a prototype rather than using \`Object.create\`
Codecov Report
@@ Coverage Diff @@
## master #3601 +/- ##
==========================================
+ Coverage 87.76% 90.87% +3.11%
==========================================
Files 673 71 -602
Lines 82837 4965 -77872
Branches 1069 1081 +12
==========================================
- Hits 72702 4512 -68190
+ Misses 10020 447 -9573
+ Partials 115 6 -109
Continue to review full report at Codecov.
|
|
@TheNeuralBit I could be wrong, but this seems like a bigger change than necessary to fix the problem? Isn't the issue with the second argument to |
|
@TheNeuralBit PR here: TheNeuralBit#2. I made the internal Row properties Symbols to avoid naming collisions, and switched the Current version of proxy-bench: This PR: |
|
@trxcllnt yeah that's a fair point, I didn't think to try I still am partial to separating out the I pushed up some commits to demonstrate it, perf seems comparable: Still needs some cleanup and a switch to Symbols if we go with it |
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.
- use Symbols for internal names
- No extra inner Row class generation
- Update comparator for symbols/clarity
e3fa246 to
2442545
Compare
|
@TheNeuralBit looks good to me 👍. last question is do we want to benchmark changing the column index lookups as Symbols? |
|
I think I'd rather merge this as is, and do that in another PR, I made ARROW-4551 to track it |
It seems that using
Object.createin thebindmethod slows down proxy generation substantially. I separated theRowandRowProxyGeneratorconcepts - now the generator creates a prototype based onRowand the parent vector when it is created, and constructs instances based on that prototype repeatedly inbind.This change improved the included benchmark from 1,114.78 ms to 154.96 ms on my laptop (Intel i5-7200U).