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.

Sign up for GitHub

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

Jump to bottom

Add Poly IC #120

Merged
merged 18 commits into from
Nov 29, 2023
Merged

Add Poly IC #120

merged 18 commits into from
Nov 29, 2023

Conversation

littledivy
Copy link
Contributor

@littledivy littledivy commented Nov 22, 2023 •
edited
Loading

Closes #116

I've condensed the self Poly inline cache implementation from https://github.com/openwebf/quickjs, plus some fixes.

Changes on top of the original patch:

  • fix test262 crash on macOS
  • code formatting
  • fix memory leak in function def
  • remove backwards bytecode compat for IC?
Benchmark poly_ic master Improvement (%)
DeltaBlue 1154 1049 10.09
RayTrace 1276 1221 4.50
RegExp 288 284 1.41
NavierStokes 2821 2767 1.95
PdfJS 4410 4323 2.01
Gameboy 10340 9355 10.49
Box2D 5174 4342 19.17
Typescript 17761 16821 5.61
Octane Score (version 9) 3347 3268 2.42

@littledivy littledivy mentioned this pull request Nov 22, 2023
Closed
@littledivy littledivy marked this pull request as draft November 22, 2023 17:58
@ammarahm-ed
Copy link
Contributor

ammarahm-ed commented Nov 23, 2023 •
edited
Loading

Great improvements in performance. I wonder what else is worth porting here 🤔 the difference in that fork is around 40% after all improvements. Is that due to using mimalloc instead or did we miss anything related to Poly IC 🤔?

@littledivy
Copy link
Contributor Author

openwebf/quickjs also implements an object prototype get/set IC. I'll try to get that working in a follow up. It also seems to be leaking memory on test262.

@ammarahm-ed
Copy link
Contributor

Got it! I was indeed having some memory issues with the fork too. I hope they are simple to fix though 😅

@littledivy littledivy marked this pull request as ready for review November 23, 2023 11:16
@littledivy
Copy link
Contributor Author

Fixed memory leaks. Should be ready for review

bnoordhuis
bnoordhuis reviewed Nov 24, 2023
View reviewed changes
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.

Bunch of comments but otherwise looks great! I'm very curious how it affects the benchmarks.

quickjs.h Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
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
Comment on lines 50574 to 50576
ch = js_malloc(ic->ctx, sizeof(InlineCacheHashSlot));
if (unlikely(!ch))
goto end;
Copy link
Contributor

Choose a reason for hiding this comment

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

For my understanding, it's safe and sound to bail out because it only results in a cache miss, no other observable changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@saghul
Copy link
Contributor

saghul commented Nov 24, 2023

I don't feel qualified to review this, so it's all yours Ben :-)

Small question though: do we need these exposed in the public API?

saghul
saghul reviewed Nov 24, 2023
View reviewed changes
quickjs.h Outdated
JSValue JS_GetPropertyInternal(JSContext *ctx, JSValueConst obj,
JSAtom prop, JSValueConst receiver,
JS_BOOL throw_ref_error);
InlineCache *ic, JS_BOOL throw_ref_error);
JSValue JS_GetPropertyInternalWithIC(JSContext *ctx, JSValueConst obj,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we / should we avoid having this in the public API?

Copy link
Contributor

Choose a reason for hiding this comment

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

This function and JS_SetPropertyInternalWithIC indeed don't have to be public.

saghul
saghul reviewed Nov 28, 2023
View reviewed changes
quickjs.h Outdated Show resolved Hide resolved
saghul
saghul reviewed Nov 28, 2023
View reviewed changes
quickjs.h Outdated
JSAtom prop, JSValueConst receiver,
JS_BOOL throw_ref_error);
JSAtom prop, JSValueConst receiver,
JSInlineCache *ic, JS_BOOL throw_ref_error);
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems no way for outside code to use the IC is there? Then how about creating some JS_GetPropertyInternal2 method on the C file which does thake the IC and leave this one alone, which would internally call JS_GetPropertyInternal2 with IC as NULL? Same for the setter.

@littledivy
Copy link
Contributor Author

Thank you for the reviews.

bnoordhuis
bnoordhuis approved these changes Nov 28, 2023
View reviewed changes
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.

LGTM except that force_inline is maybe a little overused here. Unless there's a big dip in the benchmarks, I'd remove them and leave it up to the compiler, because right now I expect it's something of a deoptimization.

That also gives you another opportunity to line up the function parameters ^_^

quickjs.c Show resolved Hide resolved
@littledivy
Copy link
Contributor Author

As you might have noticed I'm very bad at manually formatting things :)

bnoordhuis
bnoordhuis approved these changes Nov 29, 2023
View reviewed changes
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.

LGTM. Thanks, Divy!

@bnoordhuis bnoordhuis merged commit 6b78c7f into quickjs-ng:master Nov 29, 2023
@saghul
Copy link
Contributor

saghul commented Nov 29, 2023

This is awesome! Can't wait to update txiki.js tonight!

This was referenced Dec 10, 2023
Merged
Closed
bnoordhuis added a commit to bnoordhuis/quickjs that referenced this pull request Dec 14, 2023
@bnoordhuis
Remove broken JS_READ_OBJ_ROM_DATA flag
440aba4 
This JS_ReadObject() flag no longer works for bytecode. The IC opcodes
are patched during execution.

Fixes: quickjs-ng#206
Refs: quickjs-ng#120
@bnoordhuis bnoordhuis mentioned this pull request Dec 14, 2023
Merged
bnoordhuis added a commit that referenced this pull request Dec 14, 2023
@bnoordhuis
Remove broken JS_READ_OBJ_ROM_DATA flag (#216)
ba8b80f 
This JS_ReadObject() flag no longer works for bytecode. The IC opcodes
are patched during execution.

Fixes: #206
Refs: #120
@morn-0 morn-0 mentioned this pull request Mar 4, 2024
Open
@saghul saghul mentioned this pull request Feb 3, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saghul saghul saghul left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Optimization: Add support for Poly IC
4 participants
@littledivy @ammarahm-ed @saghul @bnoordhuis