Closed Bug 797825 Opened 7 years ago Closed 7 years ago

IonMonkey: Inline Array.prototype.concat

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [ion:p2])

Attachments

(1 file)

See bug 797401 comment 6 and 7.
Attached patch PatchSplinter Review
The TI code is the most complicated, but it's just like the JM+TI implementation.
Attachment #668001 - Flags: review?(dvander)
Comment on attachment 668001 [details] [diff] [review]
Patch

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

::: js/src/ion/MCallOptimize.cpp
@@ +313,5 @@
>  }
>  
>  IonBuilder::InliningStatus
> +IonBuilder::inlineArrayConcat(uint32 argc, bool constructing)
> +{

Wow, you're right: these checks are quite complicated. Is all of this to minimize the amount of guarding needed before taking the array_concat_dense path?

Seems fine for now, but very unfortunate. V8 doesn't seem to have to do any path like this, so I wonder if self-hosting could help.

::: js/src/ion/MIR.h
@@ +3754,5 @@
> +    TypePolicy *typePolicy() {
> +        return this;
> +    }
> +    AliasSet getAliasSet() const {
> +        return AliasSet::Store(AliasSet::Element | AliasSet::ObjectFields);

Are these alias sets needed? Do the input arrays get modified, or are we just creating a new array?
Attachment #668001 - Flags: review?(dvander) → review+
https://hg.mozilla.org/mozilla-central/rev/497a3ed4573a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment on attachment 668001 [details] [diff] [review]
Patch

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

::: js/src/ion/MCallOptimize.cpp
@@ +315,5 @@
>  IonBuilder::InliningStatus
> +IonBuilder::inlineArrayConcat(uint32 argc, bool constructing)
> +{
> +    if (argc != 1 || constructing)
> +        return InliningStatus_NotInlined;

argc != 1 or argc != 2?
(In reply to :Ms2ger from comment #5)
> 
> argc != 1 or argc != 2?

argc != 1 is okay to handle x.concat(y). I also verified the patch helps some micro-benchmarks and bug 797401, if this check was wrong the patch wouldn't do anything.

Thanks for checking!
You need to log in before you can comment on or make changes to this bug.