Last Comment Bug 838991 - StackTypeSet::convertDoubleElements preventing dense arrays from being compiled as such in Ion
: StackTypeSet::convertDoubleElements preventing dense arrays from being compil...
Status: RESOLVED INVALID
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: general
:
: Naveed Ihsanullah [:naveed]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-07 00:56 PST by Shu-yu Guo [:shu]
Modified: 2013-02-07 10:08 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
microbenchmark (350 bytes, application/x-javascript)
2013-02-07 00:56 PST, Shu-yu Guo [:shu]
no flags Details
patch (1.08 KB, patch)
2013-02-07 00:57 PST, Shu-yu Guo [:shu]
no flags Details | Diff | Splinter Review

Description User image Shu-yu Guo [:shu] 2013-02-07 00:56:46 PST
Created attachment 711195 [details]
microbenchmark

The current logic on |StackTypeSet::convertDoubleElements| returns |AmbiguousDoubleConversion| for a type set that includes a mix between packed arrays with an element type set of {int} and packed arrays with an element typeset of {int,float}. The {int} packed array will set |dontConvert = true| via |!types->hasType(Types::DoubleType())|, and the {int,float} packed array will set |maybeConvert = true|.

This causes a getelem IC, or worse, a VM call.

There's a ParallelArray benchmark that has a loop similar to the |sum| loop in the attached microbenchmark.

Benchmark before patch:
  % time: 57

Benchmark after patch:
  % time: 32
Comment 1 User image Shu-yu Guo [:shu] 2013-02-07 00:57:24 PST
Created attachment 711196 [details] [diff] [review]
patch
Comment 2 User image Brian Hackett (:bhackett) 2013-02-07 04:02:35 PST
Comment on attachment 711196 [details] [diff] [review]
patch

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

::: js/src/jsinfer.cpp
@@ +2047,5 @@
> +            if (!types->hasType(Type::Int32Type()))
> +                dontConvert = true;
> +            alwaysConvert = false;
> +            continue;
> +        }

This logic will allow writing doubles to an array that only contains int32s, which will both break the type information for the array and unnecessarily deoptimize it.

Can you post the benchmark and point out where the problem is at?  This has to be a setelem rather than a getelem as an AmbiguousDoubleConversion on getelem just means we can't do an optimized double load.

I think the only way to fix this is a new MIR/LIR setelem node that checks for double arrays and then either writes its int32 input or converts the input to a double first.
Comment 3 User image Shu-yu Guo [:shu] 2013-02-07 09:57:31 PST
(In reply to Brian Hackett (:bhackett) from comment #2)
> This logic will allow writing doubles to an array that only contains int32s,
> which will both break the type information for the array and unnecessarily
> deoptimize it.
> 

I see.

> Can you post the benchmark and point out where the problem is at?  This has
> to be a setelem rather than a getelem as an AmbiguousDoubleConversion on
> getelem just means we can't do an optimized double load.

The benchmark is already attached. The problem is that all the reads from a[i+j] gets compiled as GetElemCache, as |TypeInferenceOracle::elementReadIsDenseNative| returns true only if convertDoubleElements returns something other than AmbiguousDoubleConversion.

Among other things, GetElemCache isn't thread-safe.

> 
> I think the only way to fix this is a new MIR/LIR setelem node that checks
> for double arrays and then either writes its int32 input or converts the
> input to a double first.
Comment 4 User image Shu-yu Guo [:shu] 2013-02-07 10:08:53 PST
I was looking at the wrong tree. This problem is local to the ParallelArray branch.

Note You need to log in before you can comment on or make changes to this bug.