Stream: git-wasmtime

Topic: wasmtime / issue #4551 Implement an incremental compilati...

Wasmtime GitHub notifications bot (Aug 01 2022 at 15:09):

bnjbvr commented on issue #4551:

Thanks all for the first batch of reviews!

@cfallin this makes sense to me. At this point there's not much that needs those relocations, if I'm not mistaken; mostly things that weren't trivially Hashables, or source locations that were absolute, or external names being embedded and copied around. I'm looking into this.

Wasmtime GitHub notifications bot (Aug 10 2022 at 16:14):

bnjbvr commented on issue #4551:

I've updated the initial text of this PR, and while not ideal/perfect, I claim this is ready for review and could be merged as a first step. Remaining things to do as follow-ups, either myself in my copious free time, or other interested contributors :heart_eyes: :

• fuzzing could be improved, to generate an instruction that could serve as replacement of another instruction. This would likely also require a position where to put this instruction, and some care ensuring that the instruction is compatible (same number of input/returns + same types for all inputs/returns)
• there's a proliferation of Name types that could potentially be unified, maybe?
• ExternalName could just become a reference into the FunctionParameters mapping of reference to actual name value, as (it's likely, not proven that) codegen doesn't care about the identity of the function callee. So ExternalName would become a new entity type, and there'd be ExternalNameData in the table. Might not be true for libcalls which identity may matter during codegen, so may require that ExternalName becomes a sum type: either a reference into the FunctionParameter's table, or the Libcall embedded itself.
• optimization work could be done to identify why a recompilation is, in the worst case, only 50% faster than compiling from scratch (it should be much higher in theory).

Wasmtime GitHub notifications bot (Aug 10 2022 at 16:54):

bnjbvr edited a comment on issue #4551:

I've updated the initial text of this PR, and while not ideal/perfect, I claim this is ready for review and could be merged as a first step. Remaining things to do as follow-ups, either myself in my copious free time, or other interested contributors :heart_eyes: :

• fuzzing could be improved, to generate an instruction that could serve as replacement of another instruction. This would likely also require a position where to put this instruction, and some care ensuring that the instruction is compatible (same number of input/returns + same types for all inputs/returns)
• there's a proliferation of Name types that could potentially be unified, maybe?
• ExternalName could just become a reference into the FunctionParameters mapping of reference to actual name value, as (it's likely, not proven that) codegen doesn't care about the identity of the function callee. So ExternalName would become a new entity type, and there'd be ExternalNameData in the table. Might not be true for libcalls which identity may matter during codegen, so may require that ExternalName becomes a sum type: either a reference into the FunctionParameter's table, or the Libcall embedded itself.
• right now, if a UserExternalNameRef changes (that is, a function calls decides to change callee), then it changes the identity of the FunctionStencil, thus resulting in a cache miss; this could likely be refactored and improved, enabling more cache hits.
• optimization work could be done to identify why a recompilation is, in the worst case, only 50% faster than compiling from scratch (it should be much higher in theory).

Wasmtime GitHub notifications bot (Aug 10 2022 at 16:54):

bnjbvr edited a comment on issue #4551:

I've updated the initial text of this PR, and while not ideal/perfect, I claim this is ready for review and could be merged as a first step. Remaining things to do as follow-ups, either myself in my copious free time, or other interested contributors :heart_eyes: :

• fuzzing could be improved, to generate an instruction that could serve as replacement of another instruction. This would likely also require a position where to put this instruction, and some care ensuring that the instruction is compatible (same number of input/returns + same types for all inputs/returns)
• there's a proliferation of Name types that could potentially be unified, maybe?
• ExternalName could just become a reference into the FunctionParameters mapping of reference to actual name value, as (it's likely, not proven that) codegen doesn't care about the identity of the function callee. So ExternalName would become a new entity type, and there'd be ExternalNameData in the table. Might not be true for libcalls which identity may matter during codegen, so may require that ExternalName becomes a sum type: either a reference into the FunctionParameter's table, or the Libcall embedded itself.
• right now, if a UserExternalNameRef changes (that is, a function call switches the callee identity), then it changes the identity of the FunctionStencil, thus resulting in a cache miss; this could likely be refactored and improved, enabling more cache hits.
• optimization work could be done to identify why a recompilation is, in the worst case, only 50% faster than compiling from scratch (it should be much higher in theory).

Wasmtime GitHub notifications bot (Aug 11 2022 at 17:12):

bnjbvr commented on issue #4551:

@cfallin This new version incorporates changes to address your review feedback during the live review session (that we organized to help going through the changes, as this is a large patch). This is now ready and passes cargo test --all on my machine, with no performance regression on compile times, so I would claim this is ready :-) Cheers!

Wasmtime GitHub notifications bot (Aug 11 2022 at 18:27):

cfallin commented on issue #4551:

I just merged in main to this PR to see if it fixes CI.

Wasmtime GitHub notifications bot (Aug 11 2022 at 19:16):

cfallin commented on issue #4551:

Hmm, @bnjbvr it looks like some of the changes broke the fuzz targets -- happy to merge once that is fixed up.

Last updated: Nov 22 2024 at 17:03 UTC