Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Don't serialize IC opcodes #334

Merged
merged 2 commits into from
Mar 27, 2024
Merged

Conversation

bnoordhuis
Copy link
Contributor

@bnoordhuis bnoordhuis commented Mar 27, 2024

Translate IC opcodes to their non-IC variants before writing them out. Before this commit they were not byte-swapped properly, breaking the ability to load serialized bytecode containing ICs on systems with different endianness.

cc @chqrlie since it's something you mentioned IIRC

edit: note to self: mention in commit log that ICs are recomputed on the fly/as needed

@bnoordhuis
Copy link
Contributor Author

I'm going to rework this slightly and read/write the IC atoms as regular atoms in JS_ReadFunctionTag/JS_WriteFunctionTag. Right now they're stored with a distinct tag but that's not necessary anymore.

Translate IC opcodes to their non-IC variants before writing them out.
Before this commit they were not byte-swapped properly, breaking the
ability to load serialized bytecode containing ICs on systems with
different endianness. Inline caches are recomputed as needed now.

A pleasing side effect of this change is that serialized bytecode is,
on average, a little smaller because fewer atoms are duplicated now.
@chqrlie
Copy link
Collaborator

chqrlie commented Mar 27, 2024

I'm going to rework this slightly and read/write the IC atoms as regular atoms in JS_ReadFunctionTag/JS_WriteFunctionTag. Right now they're stored with a distinct tag but that's not necessary anymore.

Yes. This should remove the need for the 255 73 67 hack in the serializer I suppose. But the bytecode should be scanned after loading to create the IC slots with add_ic_slot1. Unless you make this fully transparent and create the IC slots on the fly in JS_CallInternal, which makes emit_ic useless as well

@bnoordhuis
Copy link
Contributor Author

Unless you make this fully transparent and create the IC slots on the fly in JS_CallInternal, which makes emit_ic useless as well

Not sure I follow, emit_ic is used in plenty of places. I suppose I could inline add_ic_slot1 into emit_ic because that is its only caller.

@chqrlie
Copy link
Collaborator

chqrlie commented Mar 27, 2024

Unless you make this fully transparent and create the IC slots on the fly in JS_CallInternal, which makes emit_ic useless as well

Not sure I follow, emit_ic is used in plenty of places. I suppose I could inline add_ic_slot1 into emit_ic because that is its only caller.

Well I don't understand the need for emit_ic everywhere in the compiler. Why not create the slots on the fly in add_ic_slot as the properties get actually used in the interpreter loop. This would reduce memory and time overhead for code that is never executed.

@bnoordhuis
Copy link
Contributor Author

Ah, okay. I don't disagree but that's a separate discussion.

@bnoordhuis bnoordhuis merged commit c7ca3fe into quickjs-ng:master Mar 27, 2024
38 checks passed
@bnoordhuis bnoordhuis deleted the no-ic-op-serde branch March 27, 2024 11:07
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants