1061318 - Output type of CreateRegExpMatch becomes AnyObject in octane-regexp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

So the function Exec in octane-regexp is quite hot and we want to have good code there. E.g. we expect an array from re.exec() and we should be able to find out ".length" will result in a array length lookup

Now I noticed that running re.exec() in octane-regexp will result in an AnyObject. The result in CreateRegExpMatchResult using NewDenseAllocatedArrayWithTemplate doesn't aggregate to 1 TypeObject  which is required here.

Comment 1

[Hannes Verschore [:h4writer]]

Brian, can you have a look at how we need to adjust this to have a TypeObject instead of AnyObject when aggregating multiple re.exec() results?

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

The default type for new arrays (and new plain objects) has unknown type, so this gives the regexp exec template a new type and makes sure its type information is filled in properly.

This doesn't seem to move the octane-regexp score much, if at all.  Do you know where we're spending our time on this benchmark?

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 8484393 [details] [diff] [review]
patch

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

Thanks! It was expected this wouldn't bring a major win.
But it definitely generates better code. Possibly this might have been more visible if we fixed the other things first.
Nonetheless when I first created the code I wanted it to have this type information. So glad it is fixed. 

Your question about where we spend our time:
I think it is mostly overhead. Jumping into C++ code. Checking all cornercases etc. It might be good to selfhost more of this code. (Like v8 does).
A lot of time is spend in CreateMatchResult. Having this in jit-code will improve our allocation speed.
Also v8 has this trampoline from jit to irregexp, instead of having a vm call in between, right?
This might also bring a big speed increase.

Comment 4

[Brian Hackett [Laid off!]]

Hmm, the ABI for our irregexp-generated jitcode is pretty simple and was designed so it would be relatively easy to call it directly from Ion code, so that would be good to actually do.  I might be able to work on this soon (bug 1058340 is kind of stalled right now waiting for other folks' patches to land).

Comment 5

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf48cf5e0730

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bf48cf5e0730

Comment 7

[Hannes Verschore [:h4writer]]

Comment on attachment 8484393 [details] [diff] [review]
patch

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

::: js/src/vm/RegExpObject.cpp
@@ +710,5 @@
>  
> +    // Create a new type for the template.
> +    Rooted<TaggedProto> proto(cx, templateObject->getTaggedProto());
> +    types::TypeObject *type =
> +        cx->compartment()->types.newTypeObject(cx, templateObject->getClass(), proto);

decoder made me aware I oversaw an possible issue here. We need to check if type is not a nullptr. and return if it is.