Very slow Javascript performance when computing hash in Virustotal

RESOLVED FIXED in Firefox 46

Status

()

defect
--
major
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: u552899, Assigned: nbp)

Tracking

({perf, regression})

35 Branch
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 wontfix, firefox42 wontfix, firefox43 wontfix, firefox44 wontfix, firefox45 wontfix, firefox46 fixed, firefox-esr38 wontfix, firefox-esr4546+ fixed)

Details

Attachments

(3 attachments)

When submitting a file to virustotal.com using Firefox, the initial client-side hash calculation is very slow compared to other browsers. This is especially apparent with large files.

Tested on different systems running Windows and Linux. This has been occurring on many Firefox versions. 


Example on Windows 7, 32bit, Intel Core i5-520M CPU, using Firefox 41.0.2 English (US) offline installer as a test file, size about 42 MB:

Firefox 40.0.3: ~30 seconds
Vivaldi TP 4.1.0.219.50: ~4.3 seconds
I can confirm.

The hash calculation step is slowest in Nightly. It is blazing fast in Chrome, and quite fast in Edge.
Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2f7bdbc8beb2&tochange=54a2ae5e9c18

Regressed by:Bug 1072188

Measured with 40MB file on Core2Quad power save mode:
Before landing Bug 1072188 :  3sec
After landing Bug 1072188  :  20sec
Blocks: 1072188
Status: UNCONFIRMED → NEW
Component: JavaScript Engine → JavaScript Engine: JIT
Ever confirmed: true
Flags: needinfo?(nicolas.b.pierron)
Keywords: perf, regression
Severity: normal → major
Version: unspecified → 35 Branch
The reason why this is very slow is caused by the fact the we have a MAdd which is being compiled as an Int32 + Int32, and expected to return numbers which are Int32 results.  More precisely, this is the addition line 141 for computing the value of T2.

		T2 = sha256_Sigma0(a) + majority(a, b, c);
                […]
		a = safe_add(T1, T2);

This Add instruction is flagged as being observable, because its result is used as argument of the safe_add function.  The isObservable flag of safe_add argument implies that we expect to introspect these arguments without having any computation such as RecoverInstructions.

As such, as we expect to have a truncated int32 results after the fact, the Add instruction is flagged as TruncateAfterBailout, which cause the Range Analysis to add a bailout at the location of this addition, and cause the execution to bailout.

So, we have 2 problems, the first one being how to avoid these repeated bailouts, and the second is how to avoid this isObservable flag.
This patch add a boolean flag to the JSScript in order to record the
Bailout_OverflowInvalidate used by MAdd lowering.  If this flag is set while
we are optimizing in Range Analysis, then we do not consider the
optimization level which consists of bailing out if we are not in the Int32 range.

This effectively prevent the repeated bailouts seen here and locally divide
the execution time by 8.6 (~13s -> ~1.5s).
Attachment #8680107 - Flags: review?(sunfish)
Attachment #8680107 - Flags: review?(jdemooij)
This patch is independent of the previous one, and should bring the same
performance improvement if applied alone.

This patch change the logic in ComputeRequestedTruncateKind to consider the
fact that we are now able to execute recover instruction ahead of the
bailout (since last year).

Thus, even if the instruction is captured and observable by the argument
list of the inlined function, we are still able to recover these on bailout
as long as the inline script does not need an argument object [1].

This optimize the code that we produce by avoiding the TruncateAfterBailout
flag that we used to have for the MAdd instruction, and replace it by a
fully truncated MAdd plus a cloned non-truncated MAdd instruction used for
the resume point and recover instructions.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/CompileInfo.h#545
Attachment #8680116 - Flags: review?(sunfish)
Attachment #8680116 - Flags: review?(jdemooij)
Flags: needinfo?(nicolas.b.pierron)
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Comment on attachment 8680107 [details] [diff] [review]
Range Analysis: Do not eagerly optimize with truncate-after-bailout if we bailed out with an overflow.

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

Sorry for the delay.

We should probably backport this, at least to Aurora.

::: js/src/jit/BaselineBailouts.cpp
@@ +1883,5 @@
>          break;
>  
>        // Invalid assumption based on baseline code.
>        case Bailout_OverflowInvalidate:
> +        outerScript->setHadOverflowBailout();

Add a // FALL THROUGH comment after this line.

::: js/src/jit/RangeAnalysis.cpp
@@ +3013,5 @@
>                  continue;
>  
> +            // Range Analysis is sometimes eager to do optimizations, up to the
> +            // point where we might not be able to truncate an instruction,
> +            // because its value is observable and not recoverable, and still

This is a really long sentence spanning 5 lines and it's still not very clear. Split it up?

@@ +3014,5 @@
>  
> +            // Range Analysis is sometimes eager to do optimizations, up to the
> +            // point where we might not be able to truncate an instruction,
> +            // because its value is observable and not recoverable, and still
> +            // check that the out-come of the computation is still in the

Nit: s/out-come/outcome/, or maybe better "result"

@@ +3017,5 @@
> +            // because its value is observable and not recoverable, and still
> +            // check that the out-come of the computation is still in the
> +            // integer range. This is what is implied by TruncateAfterBailout.
> +            //
> +            // If we already experienced on TruncateAfterBailout while executing

"an overflow bailout"?

@@ +3019,5 @@
> +            // integer range. This is what is implied by TruncateAfterBailout.
> +            //
> +            // If we already experienced on TruncateAfterBailout while executing
> +            // code within the current JSScript, we no longer attempt to make
> +            // this kind of eager optimizations.

s/optimizations/optimization/
Attachment #8680107 - Flags: review?(jdemooij) → review+
Comment on attachment 8680116 [details] [diff] [review]
Range Analysis: Recover observable operands if they are recoverable.

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

Looks reasonable but I'm not familiar with the truncation code so maybe sunfish has more thoughts.

Also nice speedup :) Is it on par with Chrome/V8 now?

::: js/src/jit/RangeAnalysis.cpp
@@ +2849,5 @@
>      // have to either recover the result during the bailout, or avoid the
>      // truncation.
>      if (isCapturedResult && needsConversion) {
>  
> +        // 1. These optimizations are pointless if there is no removed uses or

Nit: either "there are no removed uses" or "there is no removed use"

@@ +2870,1 @@
>          else if (hasUseRemoved || isObservableResult)

Nit: the if has {} now so the else-if should also get them, but having the comment in the middle makes it a bit unclear. What we usually do is:

} else if (hasUseRemoved || isObservableResult) {
    // ... Put the comment here ...
}
Attachment #8680116 - Flags: review?(jdemooij) → review+
Comment on attachment 8680107 [details] [diff] [review]
Range Analysis: Do not eagerly optimize with truncate-after-bailout if we bailed out with an overflow.

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

lgtm, with :jandem's comments addressed.

::: js/src/jit/CompileInfo.h
@@ +576,5 @@
>      bool scriptNeedsArgsObj_;
>  
> +    // Record the state of previous bailouts in order to prevent compiling the
> +    // same function identically the next time.
> +    bool hadOverflowBailout_:1;

This doesn't need to be a bitfield, since it's not adjacent to any other bitfields.
Attachment #8680107 - Flags: review?(sunfish) → review+
Comment on attachment 8680116 [details] [diff] [review]
Range Analysis: Recover observable operands if they are recoverable.

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

::: js/src/jit/RangeAnalysis.cpp
@@ +2850,5 @@
>      // truncation.
>      if (isCapturedResult && needsConversion) {
>  
> +        // 1. These optimizations are pointless if there is no removed uses or
> +        // any resume point observing the result.  Not having any means that we

Does this mean to say "Not having any uses?"

@@ +2858,4 @@
>          // instruction.
> +        if ((hasUseRemoved || (isObservableResult && isRecoverableResult)) &&
> +            candidate->canRecoverOnBailout())
> +        {

The logic here is subtly changed; previously, it would never set *shoudClone to true unless hasUseRemoved was true. With the patch, it sets *shouldClone to true even in some cases where hasUseRemoved is not true. Why do we care about resume points observing the result if there were no removed uses?
(In reply to Dan Gohman [:sunfish] from comment #10)
> Comment on attachment 8680116 [details] [diff] [review]
> Range Analysis: Recover observable operands if they are recoverable.
> 
> Review of attachment 8680116 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/RangeAnalysis.cpp
> @@ +2850,5 @@
> >      // truncation.
> >      if (isCapturedResult && needsConversion) {
> >  
> > +        // 1. These optimizations are pointless if there is no removed uses or
> > +        // any resume point observing the result.  Not having any means that we
> 
> Does this mean to say "Not having any uses?"

Not having any [of either removed uses or observed resume point operand uses] means that …

> @@ +2858,4 @@
> >          // instruction.
> > +        if ((hasUseRemoved || (isObservableResult && isRecoverableResult)) &&
> > +            candidate->canRecoverOnBailout())
> > +        {
> 
> The logic here is subtly changed; previously, it would never set *shoudClone
> to true unless hasUseRemoved was true. With the patch, it sets *shouldClone
> to true even in some cases where hasUseRemoved is not true. Why do we care
> about resume points observing the result if there were no removed uses?

Operands of resume points are flagged as "Observable" when it can be inspected when we iterate over the stack.  For example, formal arguments are observable operands on resume points.  This matters when we inline a function, as Range Analysis might want to optimize operations made on these arguments.

When the original code was made, a 1.5y ago, we had no way to recover observable operands, and thus this code assumes that we if the operand is directly observable by a resume point, then we cannot optimize it.

1y ago, I added a way to recover observable operands, as this was needed for removing the scope chain object with scalar replacement of objects.  This means that we can recover observable operands of the resume points (snapshots) ahead of the bailout.

With this patch, we are now optimizing "Observable" operands, the same way as any other value which has removed uses.  This is no longer an issue, as we are able to recover observable operands ahead of bailouts.  Thus, this remove the constraint that we are no longer able to optimize inline functions arguments.
Attachment #8680116 - Flags: review?(sunfish) → review+
https://hg.mozilla.org/mozilla-central/rev/02c4af508dd1
https://hg.mozilla.org/mozilla-central/rev/b4046ca29b14
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Marking 45 as affected since this fix landed originally in 46.
[Tracking Requested - why for this release]:

We would need this in esr45 in order to uplift bug 1239075. Double checking with Sylvestre that he agrees with me on that bug
Comment on attachment 8680116 [details] [diff] [review]
Range Analysis: Recover observable operands if they are recoverable.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Needed for uplifting Bug 1239075.

User impact if declined: low, performance issues.
Fix Landed on Version: Gecko 46.
Risk to taking this patch (and alternatives if risky): low, latent bugs (Bug 1239075) fixed with the help of lines added as part of this patch.
String or UUID changes made by this patch: N/A
Attachment #8680116 - Flags: approval-mozilla-esr45?
Comment on attachment 8680116 [details] [diff] [review]
Range Analysis: Recover observable operands if they are recoverable.

Taking it for bug 1239075
Attachment #8680116 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
You need to log in before you can comment on or make changes to this bug.