Preallocate MatchResult shape

RESOLVED FIXED in mozilla29

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sstangl, Assigned: h4writer)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
This is a spin-off from Bug 877021 Comment 3. Although this functionality appears to have been wanted for a while, I haven't found any bugs on file for it.

The result of calling CreateRegExpMatchResult() -- used by non-global String.match() and RegExp.exec() -- is always an array object with some amount of slots and two properties, "index" and "input".

When adding the "index" and "input" properties to the array object, we call DefineProperty(), which by Bug 877021 Comment 3 winds up spending a significant amount of time in updateSlotsForSpan().

It would be better if we could preallocate the resultant object and then merely fill in its properties without going through the new property definition paths.
Note that this is a significant cost in jQuery $("#foo") (bug 876846).
Blocks: 876846
Blocks: 940815
(Assignee)

Updated

4 years ago
Blocks: 806646
(Assignee)

Updated

4 years ago
Assignee: general → hv1989
(Assignee)

Comment 2

4 years ago
First tests show the following on octane2.0 regexp (very buggy, but working code):

1) base: 1700
2) not setting index/input: 2500 (47% improved)
3) templateObject: 1900 (12% improved)
4) fastpath setting index/input: 2275 (34% improved)

Number (1) is the score we currently have.

Number (2) is the maximum improved we can can with this optimization. Other improvements will need to happen somewhere else. This is just for reference.

Number (3) creates a templateobject and uses the type/shape of the templateobject to initialize this Array. The index/input are still set by doing "DefinePropertyHelper". This is a nice improvement.

Number (4) uses the templateobject, but adjusts the value of index/input directly in the right spot. So directly patching the right values in the slotspan. (Since we know the exact location now).
(Assignee)

Comment 3

4 years ago
Created attachment 8345626 [details] [diff] [review]
WIP

This does (4) in a mostly sane way.

Only need to figure out if there could possible a fault in the JSObject::create with not empty "extantSlots". It looks like they now always get overwriten by obj->initializeSlotRange(0, span); ?!?
Flags: needinfo?(terrence)
(Assignee)

Comment 4

4 years ago
Created attachment 8345876 [details] [diff] [review]
Patch

@Terrence: I think I interpreted extantSlots wrongly. Using another way now

@Bhackett: I did it differently than you suggested, because octane-regexp is mostly not running in ionmonkey. So doing this only in ionmonkey didn't yield all the gains.
Attachment #8345626 - Attachment is obsolete: true
Attachment #8345876 - Flags: review?(bhackett1024)
Flags: needinfo?(terrence)
Comment on attachment 8345876 [details] [diff] [review]
Patch

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

::: js/src/builtin/RegExp.cpp
@@ +86,2 @@
>      RootedValue inputVal(cx, StringValue(input));
> +    arr->nativeSetSlot(1, inputVal);

These should assert that the |index| and |input| properties ended up in the appropriate slots.

::: js/src/jsobjinlines.h
@@ +501,5 @@
>          return nullptr;
>  
> +    js::HeapSlot *slots = nullptr;
> +    size_t nDynamicSlots = dynamicSlotsCount(shape->numFixedSlots(), shape->slotSpan());
> +    if (nDynamicSlots) {

if (size_t nDynamicSlots = ...)

::: js/src/vm/RegExpObject.h
@@ +320,5 @@
>  
> +    /*
> +     * Template object where the result of .regex() is based on.
> +     */
> +    HeapPtrObject templateObject_;

How about matchResultTemplateObject_?  Can you reference CreateRegExpMatchResult here?  What is .regex()?

@@ +336,5 @@
>      /* Like 'get', but compile 'maybeOpt' (if non-null). */
>      bool get(JSContext *cx, HandleAtom source, JSString *maybeOpt, RegExpGuard *g);
>  
> +    /* Get or create the template object used to base the result of .regex() on */
> +    HeapPtrObject &getOrCreateTemplateObject(JSContext *cx);

How about getOrCreateMatchResultTemplateObject()?
Attachment #8345876 - Flags: review?(bhackett1024) → review+
Comment on attachment 8345876 [details] [diff] [review]
Patch

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

::: js/src/jsobjinlines.h
@@ +502,5 @@
>  
> +    js::HeapSlot *slots = nullptr;
> +    size_t nDynamicSlots = dynamicSlotsCount(shape->numFixedSlots(), shape->slotSpan());
> +    if (nDynamicSlots) {
> +        slots = cx->pod_malloc<js::HeapSlot>(nDynamicSlots);

Oh, also, this will leak with GGC.  Can you hoist AllocateSlots from jsobj.cpp and use it here?
(Assignee)

Comment 7

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/421def41b670
(Assignee)

Comment 8

4 years ago
(In reply to Brian Hackett (:bhackett) from comment #6)
> Comment on attachment 8345876 [details] [diff] [review]
> Patch
> 
> Review of attachment 8345876 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsobjinlines.h
> @@ +502,5 @@
> >  
> > +    js::HeapSlot *slots = nullptr;
> > +    size_t nDynamicSlots = dynamicSlotsCount(shape->numFixedSlots(), shape->slotSpan());
> > +    if (nDynamicSlots) {
> > +        slots = cx->pod_malloc<js::HeapSlot>(nDynamicSlots);
> 
> Oh, also, this will leak with GGC.  Can you hoist AllocateSlots from
> jsobj.cpp and use it here?

Oh I missed this! I'll do this now, sorry
(Assignee)

Comment 9

4 years ago
Created attachment 8346610 [details] [diff] [review]
Use allocateSlots

Like requested
Attachment #8346610 - Flags: review?(bhackett1024)
Comment on attachment 8346610 [details] [diff] [review]
Use allocateSlots

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

::: js/src/jsobj.cpp
@@ +2518,5 @@
>          }
>      }
>  
>      if (!oldCount) {
> +        obj->slots = JSObject::allocateSlots(cx, obj, newCount);

Can lose the JSObject:: here.

@@ +2525,5 @@
>          Debug_SetSlotRangeToCrashOnTouch(obj->slots, newCount);
>          return true;
>      }
>  
> +    HeapSlot *newslots = JSObject::reallocateSlots(cx, obj, obj->slots, oldCount, newCount);

Ditto.

@@ +2561,5 @@
>      }
>  
>      JS_ASSERT(newCount >= SLOT_CAPACITY_MIN);
>  
> +    HeapSlot *newslots = JSObject::reallocateSlots(cx, obj, obj->slots, oldCount, newCount);

Ditto.

::: js/src/jsobjinlines.h
@@ +434,5 @@
>      JS_ASSERT_IF(extantSlots, dynamicSlotsCount(shape->numFixedSlots(), shape->slotSpan()));
>  
> +    JSObject *obj = js_NewGCObject<js::CanGC>(cx, kind, heap);
> +    if (!obj)
> +        return nullptr;

This allocation should happen after the |slots| allocation.  Otherwise this could leave a garbage GC thing around if the slots allocation fails and various things will crash if they try to use it (e.g. via cell iters).

@@ +440,5 @@
>      js::HeapSlot *slots = extantSlots;
>      if (!slots) {
>          size_t nDynamicSlots = dynamicSlotsCount(shape->numFixedSlots(), shape->slotSpan());
>          if (nDynamicSlots) {
> +            slots = JSObject::allocateSlots(cx, obj, nDynamicSlots);

Can lose the JSObject:: here.

@@ +496,5 @@
>      uint32_t capacity = js::gc::GetGCKindSlots(kind) - js::ObjectElements::VALUES_PER_HEADER;
>  
>      JSObject *obj = js_NewGCObject<js::CanGC>(cx, kind, heap);
>      if (!obj)
>          return nullptr;

Sorry I missed it the first time around, but the same comment as in JSObject::create applies here.

@@ +500,5 @@
>          return nullptr;
>  
>      js::HeapSlot *slots = nullptr;
>      if (size_t nDynamicSlots = dynamicSlotsCount(shape->numFixedSlots(), shape->slotSpan())) {
> +        slots = JSObject::allocateSlots(cx, obj, nDynamicSlots);

Can lose the JSObject:: here.
Attachment #8346610 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 11

4 years ago
Comment on attachment 8346610 [details] [diff] [review]
Use allocateSlots

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

As discussed on IRC. Slots need to get initialized before obj, but the AllocateSlots function needs the object. Therefore "JSObject::create" has another way of doing it. Copying that should be the correct way.
Attachment #8346610 - Flags: review+ → review-
(Assignee)

Comment 12

4 years ago
r+ from Brian over IRC. This fixes the issue raised above.

https://hg.mozilla.org/integration/mozilla-inbound/rev/551bf09ad92e
https://hg.mozilla.org/mozilla-central/rev/421def41b670
https://hg.mozilla.org/mozilla-central/rev/551bf09ad92e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 950474
Depends on: 952984

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.