Pass around a DataView instead of individual kind/payload fields - #24

Note: I dropped the size of each object in memory from 24 down to 12. I’m not exactly sure why they were 24 bytes long before since kind + payload1 + payload2 = 12 bytes and payload3 = 8 bytes which only add up to 20. If I’m missing something about the memory layout, I’d be happy to correct it.

Thanks for your cooperation.

Did you check performance benchmark suite? You can check benchmark by make benchmark in IntegrationTests directory.

I found some performance regression on number interoperation.

Before

Running 'Serialization/Write JavaScript number directly' ...
done 118.31193709373474 ms
Running 'Serialization/Write JavaScript string directly' ...
done 119.49532198905945 ms
Running 'Serialization/Swift Int to JavaScript' ...
done 3457.1316990852356 ms
Running 'Serialization/Swift String to JavaScript' ...
done 5258.867609977722 ms
Running 'Object heap/Increment and decrement RC' ...
done 3061.0713909864426 ms

After

Running 'Serialization/Write JavaScript number directly' ...
done 118.58276605606079 ms
Running 'Serialization/Write JavaScript string directly' ...
done 120.5656269788742 ms
Running 'Serialization/Swift Int to JavaScript' ...
done 4948.468513011932 ms
Running 'Serialization/Swift String to JavaScript' ...
done 5090.137539982796 ms
Running 'Object heap/Increment and decrement RC' ...
done 2963.7367210388184 ms

I'm still reviewing the changes, so please wait for a while.

I’m not exactly sure why they were 24 bytes long before since kind + payload1 + payload2 = 12 bytes and payload3 = 8 bytes which only add up to 20.

The additional 4 byte is for double which is aligned by 8 bytes in C convention

I think the regression is due to the indirection of payloads access.
Why don't you pass the two payloads directly as arguments?

I think the regression is due to the indirection of payloads access.

That definitely seems to be the case! I’m not sure why they switched to passing in the DataViews but I’d be happy to revert that part

The additional 4 byte is for double which is aligned by 8 bytes in C convention

Ah, that makes sense! Since we’re not using C doubles anymore (we’re just encoding them as two ints), this shouldn’t be an issue, right?

That definitely seems to be the case! I’m not sure why they switched to passing in the DataViews but I’d be happy to revert that part

Thanks ❤️

Ah, that makes sense! Since we’re not using C doubles anymore (we’re just encoding them as two ints), this shouldn’t be an issue, right?

Yes, I think it's ok.

Ok, so I did a test where I removed all the DataView stuff (except when decoding numbers (where we need to specify endianness, I think?)) but keeping the new function style where we only pass a single pointer representing the entire JSValue rather than kind/payload1/payload2 and it’s somehow slower than all the other styles?

Ultimately I’d be fine just closing this if it has no real benefit

Hmm, the benchmark results show that decoding number from two 32bit payloads is slower than passing 20 byte payloads directly.

I think we need to optimize the payloads, but this approach seems a little difficult.

This might be a good thing to explore in the future when there’s better tooling to analyze perf but I don’t have a good rationale for this PR right now. Not really sure why I opened it in the first place. Thanks for sticking with me as I figure out exactly which things need to be brought over!