New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 5 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
HW: All
NextAction: ----
OS: All
Priority: 2
Type: FeatureRequest



Sign in to add a comment

StringCharCodeAt slower than Crankshaft

Project Member Reported by petermarshall@chromium.org, May 11 2017

Issue description

With the attached code from a node benchmark (http:check_is_http_token), turbofan is much slower than crankshaft (3-10x, depending on the string checked).

Profile from TF:

1984   67.1%   67.1%  LazyCompile: *main ...check_is_http_token.js:43:14
575   19.4%   19.5%  Builtin: StringCharCodeAt

Profile from CS:
293   81.2%   81.8%  LazyCompile: *main ...check_is_http_token.js:43:14

It looks like we just generate a call each time, which adds considerable overhead. We should discuss options around how to improve the performance, or what node could do differently in the checkIsHttpToken function.


----------------------------------------------
(function() {

  var validTokens = [
  0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // 0 - 15
  0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // 16 - 31
  0, 1, 0, 1, 1, 1, 1, 1, 0, 0, 1, 1, 0, 1, 1, 0, // 32 - 47
  1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, // 48 - 63
  0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, // 64 - 79
  1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 1, 1, // 80 - 95
  1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, // 96 - 111
  1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 0, 1, 0, // 112 - 127
  0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // 128 ...
  0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
  0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
  0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
  0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
  0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
  0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
  0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0  // ... 255
];
function checkIsHttpToken(val) {
  if (!validTokens[val.charCodeAt(0)])
    return false;
  if (val.length < 2)
    return true;
  if (!validTokens[val.charCodeAt(1)])
    return false;
  if (val.length < 3)
    return true;
  if (!validTokens[val.charCodeAt(2)])
    return false;
  if (val.length < 4)
    return true;
  if (!validTokens[val.charCodeAt(3)])
    return false;
  for (var i = 4; i < val.length; ++i) {
    if (!validTokens[val.charCodeAt(i)])
      return false;
  }
  return true;
}


  var start = performance.now();
  var key = 'server';
  for (var i = 0; i < 1e8; i++) {
    checkIsHttpToken(key);
  }
  print(performance.now() - start);
})();
 
Cc: petermarshall@chromium.org
Labels: Performance Hotlist-NodeJS
Components: Compiler
Labels: -Type-Bug Priority-1 Type-FeatureRequest
We can inline the String.prototype.charCodeAt into TurboFan again, I had that originally, but the graphs are quite huge afterwards. I'm wondering if most of the relevant strings are sequential one-byte strings, because in that case we could just specialize to that, i.e. introduce a dedicated CheckSeqOneByteString with an appropriate Type::SeqOneByteString in TurboFan.


Cc: -petermarshall@chromium.org bmeu...@chromium.org
Owner: petermarshall@chromium.org
Status: Started (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, May 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/14fa66b7a308d6c637f15cda07cb3f33a0b79fd3

commit 14fa66b7a308d6c637f15cda07cb3f33a0b79fd3
Author: Peter Marshall <petermarshall@chromium.org>
Date: Wed May 24 11:59:52 2017

[turbofan] Add SeqStringCharCodeAt operation.

Add a sequential string type to the compiler, and transform
charCodeAt on SeqString into SeqStringCharCodeAt.

SeqStringCharCodeAt can handle one and two byte strings.

Bug: v8:6391
Change-Id: I2785257522c28f3b268c9833f5313e9630cb982a
Reviewed-on: https://chromium-review.googlesource.com/509573
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45508}
[modify] https://crrev.com/14fa66b7a308d6c637f15cda07cb3f33a0b79fd3/src/compiler/effect-control-linearizer.cc
[modify] https://crrev.com/14fa66b7a308d6c637f15cda07cb3f33a0b79fd3/src/compiler/effect-control-linearizer.h
[modify] https://crrev.com/14fa66b7a308d6c637f15cda07cb3f33a0b79fd3/src/compiler/escape-analysis.cc
[modify] https://crrev.com/14fa66b7a308d6c637f15cda07cb3f33a0b79fd3/src/compiler/opcodes.h
[modify] https://crrev.com/14fa66b7a308d6c637f15cda07cb3f33a0b79fd3/src/compiler/simplified-lowering.cc
[modify] https://crrev.com/14fa66b7a308d6c637f15cda07cb3f33a0b79fd3/src/compiler/simplified-operator.cc
[modify] https://crrev.com/14fa66b7a308d6c637f15cda07cb3f33a0b79fd3/src/compiler/simplified-operator.h
[modify] https://crrev.com/14fa66b7a308d6c637f15cda07cb3f33a0b79fd3/src/compiler/typer.cc
[modify] https://crrev.com/14fa66b7a308d6c637f15cda07cb3f33a0b79fd3/src/compiler/types.cc
[modify] https://crrev.com/14fa66b7a308d6c637f15cda07cb3f33a0b79fd3/src/compiler/types.h
[modify] https://crrev.com/14fa66b7a308d6c637f15cda07cb3f33a0b79fd3/src/compiler/verifier.cc
[modify] https://crrev.com/14fa66b7a308d6c637f15cda07cb3f33a0b79fd3/test/cctest/test-types.cc
[modify] https://crrev.com/14fa66b7a308d6c637f15cda07cb3f33a0b79fd3/test/mjsunit/string-charcodeat.js

Project Member

Comment 6 by bugdroid1@chromium.org, May 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/9d8bd0551657d60c66b2f96544eecedfba1cba8a

commit 9d8bd0551657d60c66b2f96544eecedfba1cba8a
Author: bmeurer <bmeurer@chromium.org>
Date: Wed May 24 13:53:40 2017

[turbofan] Speculatively optimize string character access.

Add a protector cell for string bounds checks that is being used to
protect speculative bounds for String.prototype.charCodeAt and
String.prototype.charAt in TurboFan (and Crankshaft). This way we don't
have the diamond in optimized code, which stands in the way of other
optimizations for charCodeAt that are currently being worked on by
petermarshall@.

BUG=v8:6391
TBR=mlippautz@chromium.org
R=petermarshall@chromium.org

Review-Url: https://codereview.chromium.org/2905623003
Cr-Commit-Position: refs/heads/master@{#45514}

[modify] https://crrev.com/9d8bd0551657d60c66b2f96544eecedfba1cba8a/src/builtins/builtins-string-gen.cc
[modify] https://crrev.com/9d8bd0551657d60c66b2f96544eecedfba1cba8a/src/compiler/js-builtin-reducer.cc
[modify] https://crrev.com/9d8bd0551657d60c66b2f96544eecedfba1cba8a/src/crankshaft/hydrogen.cc
[modify] https://crrev.com/9d8bd0551657d60c66b2f96544eecedfba1cba8a/src/heap/heap.cc
[modify] https://crrev.com/9d8bd0551657d60c66b2f96544eecedfba1cba8a/src/heap/heap.h
[modify] https://crrev.com/9d8bd0551657d60c66b2f96544eecedfba1cba8a/src/isolate-inl.h
[modify] https://crrev.com/9d8bd0551657d60c66b2f96544eecedfba1cba8a/src/isolate.h
[add] https://crrev.com/9d8bd0551657d60c66b2f96544eecedfba1cba8a/test/mjsunit/regress/regress-6391.js

Project Member

Comment 7 by bugdroid1@chromium.org, May 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/a07218a56d51698b25dedbab1038f0b6865acfd2

commit a07218a56d51698b25dedbab1038f0b6865acfd2
Author: machenbach <machenbach@chromium.org>
Date: Wed May 24 17:04:44 2017

Revert of [turbofan] Speculatively optimize string character access. (patchset #1 id:1 of https://codereview.chromium.org/2905623003/ )

Reason for revert:
Changes layout tests:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/builds/15867

See:
https://github.com/v8/v8/wiki/Blink-layout-tests

Original issue's description:
> [turbofan] Speculatively optimize string character access.
>
> Add a protector cell for string bounds checks that is being used to
> protect speculative bounds for String.prototype.charCodeAt and
> String.prototype.charAt in TurboFan (and Crankshaft). This way we don't
> have the diamond in optimized code, which stands in the way of other
> optimizations for charCodeAt that are currently being worked on by
> petermarshall@.
>
> BUG=v8:6391
> TBR=mlippautz@chromium.org
> R=petermarshall@chromium.org
>
> Review-Url: https://codereview.chromium.org/2905623003
> Cr-Commit-Position: refs/heads/master@{#45514}
> Committed: https://chromium.googlesource.com/v8/v8/+/9d8bd0551657d60c66b2f96544eecedfba1cba8a

TBR=petermarshall@chromium.org,mlippautz@chromium.org,bmeurer@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:6391

Review-Url: https://codereview.chromium.org/2900333002
Cr-Commit-Position: refs/heads/master@{#45521}

[modify] https://crrev.com/a07218a56d51698b25dedbab1038f0b6865acfd2/src/builtins/builtins-string-gen.cc
[modify] https://crrev.com/a07218a56d51698b25dedbab1038f0b6865acfd2/src/compiler/js-builtin-reducer.cc
[modify] https://crrev.com/a07218a56d51698b25dedbab1038f0b6865acfd2/src/crankshaft/hydrogen.cc
[modify] https://crrev.com/a07218a56d51698b25dedbab1038f0b6865acfd2/src/heap/heap.cc
[modify] https://crrev.com/a07218a56d51698b25dedbab1038f0b6865acfd2/src/heap/heap.h
[modify] https://crrev.com/a07218a56d51698b25dedbab1038f0b6865acfd2/src/isolate-inl.h
[modify] https://crrev.com/a07218a56d51698b25dedbab1038f0b6865acfd2/src/isolate.h
[delete] https://crrev.com/cdd9ed0879a6a33ab88278bdd6aafbf5e850d04f/test/mjsunit/regress/regress-6391.js

Project Member

Comment 8 by bugdroid1@chromium.org, May 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/481ea63d41cecab3d1a6a6bad82a069402bccdc2

commit 481ea63d41cecab3d1a6a6bad82a069402bccdc2
Author: Peter Marshall <petermarshall@chromium.org>
Date: Mon May 29 09:03:22 2017

[turbofan] Add CheckSeqString so that we have type info for CharCodeAt.

Bug: v8:6391
Change-Id: If63078c756d9cfb00e515fae005755c4ed8b12f7
Reviewed-on: https://chromium-review.googlesource.com/512803
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45549}
[modify] https://crrev.com/481ea63d41cecab3d1a6a6bad82a069402bccdc2/src/compiler/effect-control-linearizer.cc
[modify] https://crrev.com/481ea63d41cecab3d1a6a6bad82a069402bccdc2/src/compiler/effect-control-linearizer.h
[modify] https://crrev.com/481ea63d41cecab3d1a6a6bad82a069402bccdc2/src/compiler/js-builtin-reducer.cc
[modify] https://crrev.com/481ea63d41cecab3d1a6a6bad82a069402bccdc2/src/compiler/js-native-context-specialization.cc
[modify] https://crrev.com/481ea63d41cecab3d1a6a6bad82a069402bccdc2/src/compiler/opcodes.h
[modify] https://crrev.com/481ea63d41cecab3d1a6a6bad82a069402bccdc2/src/compiler/redundancy-elimination.cc
[modify] https://crrev.com/481ea63d41cecab3d1a6a6bad82a069402bccdc2/src/compiler/simplified-lowering.cc
[modify] https://crrev.com/481ea63d41cecab3d1a6a6bad82a069402bccdc2/src/compiler/simplified-operator.cc
[modify] https://crrev.com/481ea63d41cecab3d1a6a6bad82a069402bccdc2/src/compiler/simplified-operator.h
[modify] https://crrev.com/481ea63d41cecab3d1a6a6bad82a069402bccdc2/src/compiler/typed-optimization.cc
[modify] https://crrev.com/481ea63d41cecab3d1a6a6bad82a069402bccdc2/src/compiler/typed-optimization.h
[modify] https://crrev.com/481ea63d41cecab3d1a6a6bad82a069402bccdc2/src/compiler/typer.cc
[modify] https://crrev.com/481ea63d41cecab3d1a6a6bad82a069402bccdc2/src/compiler/verifier.cc

Cc: -jarin@chromium.org petermarshall@chromium.org
Labels: -Priority-1 Priority-2
Owner: jarin@chromium.org
The StringCharCodeAt regression is repaired. The benchmark still runs faster in CS because it just hoists all the charCodeAt(constant) accesses out of the outer loop, and so it doesn't do a lot of work inside the loop. TurboFan currently doesn't peel the outer loop, so we cannot eliminate any redundant accesses from the outer loop. Assigning to jarin@ to decide whether to do the outer loop peeling as well at some point.
Any update on this / can we close?

Sign in to add a comment