Skip to content
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

Add prototype inline cache #551

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

ErosZy
Copy link

@ErosZy ErosZy commented Sep 25, 2024

I add prototype inline cache for basic get/set op, there are benchmark results:

  • Apple M1 Pro
  • macOS 14.4.1
  • Clang 15.0.0 arm64-apple-darwin23.4.0
prototype-inline-cache master Improvement(%)
Richards 1363 1263 +7.91
DeltaBlue 1346 1197 +12.44
Crypto 1258 1227 +2.52
RayTrace 1370 1307 +4.82
EarleyBoyer 2340 2344 -0.29
RegExp 277 270 +2.59
Splay 2537 2307 +9.96
SplayLatency 9276 9214 +0.67
NavierStokes 2251 2197 +2.45
PdfJS 4345 4265 +1.88
Mandreel 1055 1031 +2.32
MandreelLatency 7163 7081 +1.15
Gameboy 9057 9034 +0.25
CodeLoad 17284 16973 +0.19
Box2D 5433 5234 +3.80
Typescript 17650 17766 -0.65
Score (version 9) 3041 2944 +3.29
  • Intel i5-12400F
  • Windows 10 - mingw
  • GCC 13.2.0
prototype-inline-cache master Improvement(%)
Richards 845 686 +23.17
DeltaBlue 679 575 +18,08
Crypto 801 841 -2.52
RayTrace 450 422 +4.75
EarleyBoyer 871 884 -1.47
RegExp 161 157 +2.54
Splay 950 945 +0.52
SplayLatency 4743 4564 +3.92
NavierStokes 1243 1240 +0.24
PdfJS 1942 1839 +5.60
Mandreel 532 546 -2.56
MandreelLatency 3655 3553 +1.15
Gameboy 4671 4336 +2.87
CodeLoad 8613 8485 +1.50
Box2D 2552 2171 +17.54
Typescript 8892 8379 +6.12
Score (version 9) 1485 1411 +5.24
  • Microsoft AMD-V(CPU MHz: 3244.657)
  • Linux 6.8.0-1014-azure
  • gcc (Ubuntu 9.4.0-1ubuntu1~20.04.2) 9.4.0
prototype-inline-cache master Improvement(%)
Richards 750 657 +14.15
DeltaBlue 746 604 +23.50
Crypto 614 603 +1.82
RayTrace 834 840 -0.71
EarleyBoyer 1216 1256 -3.18
RegExp 202 202 +0.00
Splay 1680 1618 +3.83
SplayLatency 5634 4945 +13.93
NavierStokes 1022 1004 +1.79
PdfJS 2369 2246 +5.47
Mandreel 478 451 +5.98
MandreelLatency 3408 3110 +9.58
Gameboy 4303 3928 +9.54
CodeLoad 11991 11628 +3.12
Box2D 2535 2332 +8.70
Typescript 7862 7934 -0.90
Score (version 9) 1639 1548 +6.84

@saghul
Copy link
Contributor

saghul commented Sep 25, 2024

Nice!

@ErosZy
Copy link
Author

ErosZy commented Sep 26, 2024

@saghul follow #244, I've also added the benchmark test results for Linux and Windows MinGW here for reference.

@saghul
Copy link
Contributor

saghul commented Sep 26, 2024

Thank you! Not sure I am qualified 😅 but I'll give it a try!

@saghul
Copy link
Contributor

saghul commented Sep 26, 2024

@bnoordhuis Could you PTAL? LGTM in so far as I can read it and the tests pass 😅

@bnoordhuis
Copy link
Contributor

I'll try to take a look over the weekend.

@chqrlie
Copy link
Collaborator

chqrlie commented Sep 27, 2024

@bnoordhuis Could you PTAL? LGTM in so far as I can read it and the tests pass 😅

I shall take a look over the week-end too. I have an alternate version on the back burner so let's see if @ErosZy approach is promissing...

quickjs.c Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
@ErosZy
Copy link
Author

ErosZy commented Oct 9, 2024

@bnoordhuis @saghul I've made some changes to the code based on the review; could you please take another look? Thanks.

Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good but I still have a few comments.

quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
@bnoordhuis
Copy link
Contributor

@ErosZy heads up, I'm doing some IC refactoring in #609 because I'm fairly sure I've identified some bugs. You may have to rework your changes a little.

(I wouldn't normally push such changes when I know they conflict with pull requests from contributors but bug fixes trump enhancements.)

@p-bakker
Copy link

Any chance in this one being finished and included in the forthcoming release? Any performance bump is always great to have 🙂

@saghul saghul marked this pull request as draft November 28, 2024 09:27
@saghul
Copy link
Contributor

saghul commented Nov 28, 2024

Converting to draft while the feedback is addressed. It also needs a rebase...

@ErosZy
Copy link
Author

ErosZy commented Nov 28, 2024

@saghul @bnoordhuis please check, thx.

@saghul saghul marked this pull request as ready for review November 28, 2024 20:37
@saghul
Copy link
Contributor

saghul commented Nov 29, 2024

Tests are 💚 ! @bnoordhuis can you PTAL?

@ErosZy
Copy link
Author

ErosZy commented Nov 29, 2024

add a screenshot of the benchmark, and after merging with the latest master branch, it still maintains a good performance improvement. It did not cause performance degradation. 😊
391118611-7a89d8a0-b583-4063-b14d-a38987eb6336

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants