New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Equality for Chars is slow due to `new String(x)` #911

Closed
zwilias opened this Issue Sep 29, 2017 · 5 comments

Comments

Projects
None yet
4 participants
@zwilias
Member

zwilias commented Sep 29, 2017

In the native parts, notable Utils.chr, the String object constructor (new String(x)) is used. This forces the result to be a boxed string, meaning - as far as the JavaScript runtime is concerned - it is an object describing a String, rather than a primitive String.

This makes equality checks on chars produced by that function extremely slow.

According to this benchmark, equality check for 2 chars is ~100x slower than equality check for 2 strings.

The root cause of that is twofold:

  • new String('x') !== new String('x'), which means that the short-circuit in eqHelp is skipped.
  • typeof (new String('x')) === 'object'; which means eqHelp will take the slow path and check for equality as if it were a record.

I can't think of any downsides to using the String primitive constructor instead (return String(x)). It means equal chars can be checked using ===, and calling String methods should be equally fast - if not faster - as compared to the StringObject.

Following the git blame history, it appears as if chars used to be marked val.isChar = true at runtime, which may be why it used to be an object, but since the disambiguation happens solely at the type level now, new should be removed.

Maybe Utils.chr can actually be removed altogether, I'm not sure what it's purpose is at this point.

@process-bot

This comment has been minimized.

Show comment
Hide comment
@process-bot

process-bot Sep 29, 2017

Thanks for the issue! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

process-bot commented Sep 29, 2017

Thanks for the issue! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

@zwilias zwilias changed the title from `new String(x)` vs `String(x)` to Equality for Chars is slow due to `new String(x)` Sep 29, 2017

@jxxcarlson

This comment has been minimized.

Show comment
Hide comment
@jxxcarlson

jxxcarlson Sep 30, 2017

Just a note re the app I am working on. This issue creates a factor-of-700 slowdown running some critical parser/rendering code "for real": 625 milliseconds in elm-test versus 8 seconds in the app

jxxcarlson commented Sep 30, 2017

Just a note re the app I am working on. This issue creates a factor-of-700 slowdown running some critical parser/rendering code "for real": 625 milliseconds in elm-test versus 8 seconds in the app

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Sep 30, 2017

Member

It is used by toString to distinguish between "a" and 'a' when showing things.

I would like to make Debug.toString only available in development, which means we could just treat them as strings in production mode.

Member

evancz commented Sep 30, 2017

It is used by toString to distinguish between "a" and 'a' when showing things.

I would like to make Debug.toString only available in development, which means we could just treat them as strings in production mode.

@zwilias

This comment has been minimized.

Show comment
Hide comment
@zwilias

zwilias Sep 30, 2017

Member

In that case, there is the option of adding yet another case to eqHelp, perhaps only during development:

    if (x instanceof String) {
      return x.valueOf() === y.valueOf();
    }

In a quick benchmark, this means 'a' == 'b' would only be ~13% slower than "a" == "b":

image
(in the screenshot, instanceof means it's the 0.18 core implementation with the above snippet added right after the === check)

Member

zwilias commented Sep 30, 2017

In that case, there is the option of adding yet another case to eqHelp, perhaps only during development:

    if (x instanceof String) {
      return x.valueOf() === y.valueOf();
    }

In a quick benchmark, this means 'a' == 'b' would only be ~13% slower than "a" == "b":

image
(in the screenshot, instanceof means it's the 0.18 core implementation with the above snippet added right after the === check)

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Mar 7, 2018

Member

In the development build, the representation is JS strings when in PROD and is the boxed version when in DEBUG to make Debug.toString work right. Probably it can be done without runtime overhead in the future, but that's the thing for now.

Member

evancz commented Mar 7, 2018

In the development build, the representation is JS strings when in PROD and is the boxed version when in DEBUG to make Debug.toString work right. Probably it can be done without runtime overhead in the future, but that's the thing for now.

@evancz evancz closed this Mar 7, 2018

@evancz evancz added the performance label Mar 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment