Closed Bug 1130178 Opened 7 years ago Closed 7 years ago

Avoid writing UndefinedValue() in ArgumentsObject::create().

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: sstangl, Assigned: sstangl)

Details

Attachments

(1 file)

ArgumentsObject::create() shows up minorly in a profile of Shumway startup, taking about 7% of the time. If we avoid writing UndefinedValue() to make the object attachable, it drops off the profile, which is nice. But maybe it's just not getting sampled anymore -- who knows? The speedup is reproducible.

This patch changes ::create() to just use pod_calloc(), which causes all the default argument Values to be read as DoubleValue(+0.0). Since that's a primitive, it's just as safe as UndefinedValue().

Really grasping at straws to fix Shumway startup time, apparently.
Attachment #8560096 - Flags: review?(sphink)
Comment on attachment 8560096 [details] [diff] [review]
0001-Avoid-a-set-loop-in-ArgumentsObject-create.patch

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

Sseems legit to me.

::: js/src/vm/ArgumentsObject.cpp
@@ +210,5 @@
>  
> +    // Attach the argument object.
> +    // Because the argument object was zeroed by pod_calloc(), each Value in
> +    // ArgumentsData is DoubleValue(0) and therefore safe for GC tracing.
> +    MOZ_ASSERT(DoubleValue(0).asRawBits() == 0x0);

In a perfect world, that would be static_assert instead of MOZ_ASSERT, but I don't suppose we can push constexprs that far yet.
Attachment #8560096 - Flags: review?(sphink) → review+
(In reply to Sean Stangl [:sstangl] from comment #0)
> This patch changes ::create() to just use pod_calloc(), which causes all the
> default argument Values to be read as DoubleValue(+0.0). Since that's a
> primitive, it's just as safe as UndefinedValue().

Nice finding!

We should definitely do the same for object initialization.  We have many cases where we have to go through a pre-initialization to make this safe for the GC, as the initialization is not-GC free.  With such trick, we can remove all these pre-initialization loops.
FWIW, I noticed Shumway has a few functions that use fun.asApply instead of fun.apply, breaking our arguments optimization (I'll file a Shumway bug for this). It might be interesting to see where we create all these arguments objects, it could be the same issue or something similar.
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> We should definitely do the same for object initialization.  We have many
> cases where we have to go through a pre-initialization to make this safe for
> the GC, as the initialization is not-GC free.  With such trick, we can
> remove all these pre-initialization loops.

Note that this will be an API change at least as regards slots and their initial value, currently guaranteed to be UndefinedValue().  You will definitely break some code if you do this.  At a minimum I'm aware of the various finalizers in builtin/Intl.cpp needing to be fixed if this change occurs.
https://hg.mozilla.org/mozilla-central/rev/5cfc8f7bd7fa
Assignee: nobody → sstangl
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.