-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
Implement Inline Caching
#2767
Implement Inline Caching
#2767
Conversation
40c7d4a
to
8f6ec97
Compare
8f6ec97
to
e561aaf
Compare
e561aaf
to
eadc11e
Compare
Benchmark for 621c245Click to view benchmark
|
eadc11e
to
4a403c4
Compare
Test262 conformance changes
|
Benchmark for cf2d077Click to view benchmark
|
4a403c4
to
604489f
Compare
Benchmark for e8a1b4aClick to view benchmark
|
604489f
to
4ebb6a2
Compare
Benchmark for 7ef3514Click to view benchmark
|
There is still a lot of work to be done (currently only the Main Richards: 19.5
DeltaBlue: 20.1
RayTrace: 75.0
----
Score (version 7): 30.8
undefined With Inline caching Richards: 25.3
DeltaBlue: 30.5
RayTrace: 101
----
Score (version 7): 42.7
undefined This is where #1605 really pays off, There are clear performance improvements. @jedel1043 I was curious in the benchmarks in (#2790 (comment)) did you edit the file, because when I ran EDIT: full benchmarks: Main
PR
Hmmm. there seems to be a bug, that is causing |
In my case it seemed like it froze, but it was just the long execution time. The only benchmark that threw errors was RegExp I think. |
Benchmark for 703d5feClick to view benchmark
|
I guess I just didn't wait long enough 😅 , I'll run them and update the comment once they finish :) |
Benchmark for ca274e0Click to view benchmark
|
Benchmark for b6de08bClick to view benchmark
|
fd93cd5
to
42acaa5
Compare
Benchmark for 54852e9Click to view benchmark
|
9ebac0a
to
509c580
Compare
f54df56
to
82e67e6
Compare
82e67e6
to
bda8fcd
Compare
Benchmark for 6ab0926Click to view benchmark
|
bda8fcd
to
37b1a64
Compare
Benchmark for 1a0328eClick to view benchmark
|
37b1a64
to
8365e91
Compare
Benchmark for 0224993Click to view benchmark
|
Just throwing some ideas into the mix. |
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 really like the feature and the changes. All seems very logical.
This should not hold up the merge, but I think it would be very nice if we could test this feature. Due to all the different components that are required to make it work, it may be that some future work accidentally could change some vital logic. I'm not 100% sure how to best test it, but maybe you have something in mind @HalidOdat
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.
Forgot to approve this. Really nice optimization work!
Also, agreed with Raskad that this should be tested somehow, but that can be done in a separate PR.
You mean having an a struct for functions (like we do for internal object methods) that contains If that's the case than I totally agree. The |
Yes! That would make it easier to preserve data between calls, and it could enable interesting optimizations. |
8365e91
to
2ff6c51
Compare
Benchmark for 5a64da3Click to view benchmark
|
Depends on #2723This PR i
currently just a POCimplements inline caching.This merges into #2723 so the changes can be easily seen, will switch tomain
, onceHidden classes
gets merged.Because theThe inline caching is now internal method aware so it knows what can be cached.Proxy
internal methods are not implement this will introduce a lot of panics ( will fix ), Was curious about the benchmarks since, we don't have proxys in the benchmarks they shouldn't fail.It changes the following:
Proxy
internal methodsGetPropertyByName
SetPropertyByName
Add inline caching toGetName
Add inline caching toSetName