Page MenuHomePhabricator

[WebAssembly] Use StringSaver to retain ownership of ctor function body. NFC

Authored by ncw on Mar 6 2018, 6:22 AM.




This makes it easier to create other synthetic functions, which don't need the SyntheticFunction instance to own the body as a std::string.

I should have done this as part of D43933, but at the time I thought what I was doing was better... oh well.

Diff Detail


Event Timeline

ncw created this revision.Mar 6 2018, 6:22 AM
sbc100 accepted this revision.Mar 6 2018, 12:07 PM
sbc100 added inline comments.
159 ↗(On Diff #137185)

Why add getComdat() here? If we really want this to be NFC maybe this part should be separate.

This revision is now accepted and ready to land.Mar 6 2018, 12:07 PM
ruiu added inline comments.Mar 6 2018, 12:58 PM
877 ↗(On Diff #137185)

I don't think the amount of data we need to copy is large, so it is presumably OK, and I'm not obsessed with micro-optimization, but here we make a copy of std::string here and then immediately discard the original string at the end of the scope. So this makes a redundant copy of a string.

There is a way to avoid unnecessary copying. If you allocate std::string by make<std::string>(), then its body will have the same lifetime as the entire linking process.

ncw added inline comments.Mar 6 2018, 2:11 PM
159 ↗(On Diff #137185)

Oh ah - that slipped in. It is NFC because getComdat() currently isn't called (it actually crashes currently), so it can't be changing any existing behaviour that doesn't crash. That chunk just got pulled in "under the radar" because it's next the lines I picked into this PR, sorry. It's necessary for the synthetic undefined stubs.

I could put it in another commit, but it kind of fits in with the "fixes to SyntheticFunction" theme.

877 ↗(On Diff #137185)

True, we could save on that copy. But if we were micro-optimising, make<std::string> would be a bit wasteful too, since make<> would have to create a whole new slab/arena for the std::string (and register the arena so the string's dtor is called).

Best of all we'd maybe do BAlloc.Allocate<char>(5 + 5 + InitFunctions.size()*6 + 1) and then write directly into the interned buffer right away, with no copies at all.

But I agree it's not worth trying to optimise; this commit is just trying to make the SyntheticFunction ctor more generally useful.

Thanks for the review!

This revision was automatically updated to reflect the committed changes.