Add Poly IC#120
Conversation
|
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 |
|
|
|
Got it! I was indeed having some memory issues with the fork too. I hope they are simple to fix though 😅 |
|
Fixed memory leaks. Should be ready for review |
bnoordhuis
left a comment
There was a problem hiding this comment.
Bunch of comments but otherwise looks great! I'm very curious how it affects the benchmarks.
| ch = js_malloc(ic->ctx, sizeof(InlineCacheHashSlot)); | ||
| if (unlikely(!ch)) | ||
| goto end; |
There was a problem hiding this comment.
For my understanding, it's safe and sound to bail out because it only results in a cache miss, no other observable changes?
|
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? |
| JSAtom prop, JSValueConst receiver, | ||
| JS_BOOL throw_ref_error); | ||
| InlineCache *ic, JS_BOOL throw_ref_error); | ||
| JSValue JS_GetPropertyInternalWithIC(JSContext *ctx, JSValueConst obj, |
There was a problem hiding this comment.
Can we / should we avoid having this in the public API?
There was a problem hiding this comment.
This function and JS_SetPropertyInternalWithIC indeed don't have to be public.
| JSAtom prop, JSValueConst receiver, | ||
| JS_BOOL throw_ref_error); | ||
| JSAtom prop, JSValueConst receiver, | ||
| JSInlineCache *ic, JS_BOOL throw_ref_error); |
There was a problem hiding this comment.
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.
|
Thank you for the reviews. |
bnoordhuis
left a comment
There was a problem hiding this comment.
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 ^_^
|
As you might have noticed I'm very bad at manually formatting things :) |
|
This is awesome! Can't wait to update txiki.js tonight! |
This JS_ReadObject() flag no longer works for bytecode. The IC opcodes are patched during execution. Fixes: quickjs-ng#206 Refs: quickjs-ng#120
This JS_ReadObject() flag no longer works for bytecode. The IC opcodes are patched during execution. Fixes: quickjs-ng/quickjs#206 Refs: quickjs-ng/quickjs#120
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:
remove backwards bytecode compat for IC?