Closed Bug 1468449 Opened Last year Closed Last year

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

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

Details

Attachments

(2 files)

No description provided.
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.
Attachment #8985118 - Flags: review?(jcoppeard)
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.
Attachment #8985119 - Flags: review?(jcoppeard)
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.
Attachment #8985118 - Flags: review?(jcoppeard) → review+
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.
Attachment #8985119 - Flags: review?(jcoppeard) → review+
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.)
Pushed by jwalden@mit.edu:
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
https://hg.mozilla.org/mozilla-central/rev/9b3683e49b73
https://hg.mozilla.org/mozilla-central/rev/bace341bdc89
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.