1468449 - Copy over BindingNames in a parser-lifetime Data to a long-lived Scope in a C++ object model-respecting way

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Use std::uninitialized_copy to implement FreshlyInitializeBindings

This isn't a semantic change, the standard algorithm does the identical thing, just a use-standard-stuff-more thing.  I *could* use the std::* function directly at use sites, but I think having a nice name is better.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Use std::uninitialized_copy_n to copy over BindingNames in a parser-lifetime Data to a long-lived Scope

Also a nice line-loss here by commoning up a bunch of silliness.  The base-class goo is totally just to give a way to verify the change isn't silently switching behavior on someone without realizing it -- I kind of like keeping it for the long term for the searchability, but it's not strictly necessary, to be sure.

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 8985118 [details] [diff] [review]
Use std::uninitialized_copy to implement FreshlyInitializeBindings

Review of attachment 8985118 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ -1777,5 @@
>   * Copy-construct |BindingName|s from |bindings| into |cursor|, then return
>   * the location one past the newly-constructed |BindingName|s.
>   */
>  static MOZ_MUST_USE BindingName*
>  FreshlyInitializeBindings(BindingName* cursor, const Vector<BindingName>& bindings)

I had to look up what this did, but yes this makes sense.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8985119 [details] [diff] [review]
Use std::uninitialized_copy_n to copy over BindingNames in a parser-lifetime Data to a long-lived Scope

Review of attachment 8985119 [details] [diff] [review]:
-----------------------------------------------------------------

Nice to get rid of all that duplication.

::: js/src/vm/Scope.cpp
@@ +153,5 @@
>              cx->markAtom(name);
>      }
>  
> +    size_t size = SizeOfData<typename ConcreteScope::Data>(data->length);
> +    void* bytes = cx->zone()->pod_malloc<char>(size);

We should maybe be using pod_malloc_with_extra<ConcreteScope> here and below.  Although there doesn't seem to be an equivalent when doing LIFO alloc in NewEmptyBinding data.

Comment 5

[Jeff Walden [:Waldo]]

FWIW I hope to remove pod_malloc_with_extra eventually -- it's a super-rare thing used almost nowhere, and it doesn't really much fit with the language.  So I don't plan to change in that manner.

Also all this pod_malloc stuff we basically just ass-u-me that it returns a pointer suitably aligned to store T, which is not particularly pleasant.  I expect to change us, over time, to a system where AllocPolicy includes C++ perfect forwarding support to *construct* objects, not merely allocate a hunk of memory, so that it's actually possible to guarantee proper alignment, and so that uninitialized memory is never exposed.  (We need to do more calling of C++ constructors and placement-new, and less of just reinterpret_cast<>-ing memory however we think we can -- not least because the latter is not actually compatible with the C++ object model.)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b3683e49b73
Use std::uninitialized_copy to implement FreshlyInitializeBindings.  r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/bace341bdc89
Use std::uninitialized_copy_n to copy over BindingNames in a parser-lifetime Data to a long-lived Scope.  r=jonco

Comment 7

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/9b3683e49b73
https://hg.mozilla.org/mozilla-central/rev/bace341bdc89