Skip to content
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

src: fix performance regression in node_file.cc #31343

Closed
wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

Commits dcb6929, 4396beb and 8a96d05 turned the O(n) scan in
InternalModuleReadJSON() into an O(4n) scan. Fix the performance
regression by turning that into a linear scan again.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Jan 13, 2020
@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see this, thanks.

src/node_file.cc Outdated

while (p < pe) {
char c = *p++;
if (c == '"') goto quote;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you use if statements here to avoid goto?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, for two reasons:

  1. Aesthetic. It keeps the level of indent small.
  2. Performance. g++ 9.2.1 at -O3 doesn't move the block out of the way. The goto keeps the inner loop at around 80 bytes:
    10ec:       80 f9 22                cmp    $0x22,%cl
    10ef:       74 1f                   je     1110 <main+0x70>
    10f1:       80 f9 5c                cmp    $0x5c,%cl
    10f4:       75 44                   jne    113a <main+0x9a>
    10f6:       48 39 fa                cmp    %rdi,%rdx
    10f9:       73 65                   jae    1160 <main+0xc0>
    10fb:       0f b6 4e 01             movzbl 0x1(%rsi),%ecx
    10ff:       80 f9 22                cmp    $0x22,%cl
    1102:       74 7c                   je     1180 <main+0xe0>
    1104:       48 89 d6                mov    %rdx,%rsi
    1107:       48 8d 56 01             lea    0x1(%rsi),%rdx
    110b:       80 f9 22                cmp    $0x22,%cl
    110e:       75 e1                   jne    10f1 <main+0x51>
    1110:       49 8d 48 08             lea    0x8(%r8),%rcx
    1114:       49 89 10                mov    %rdx,(%r8)
    1117:       4c 39 c9                cmp    %r9,%rcx
    111a:       72 34                   jb     1150 <main+0xb0>
    111c:       48 8b 34 24             mov    (%rsp),%rsi
    1120:       48 89 f1                mov    %rsi,%rcx
    1123:       48 f7 d1                not    %rcx
    1126:       48 03 4c 24 08          add    0x8(%rsp),%rcx
    112b:       48 83 f9 04             cmp    $0x4,%rcx
    112f:       74 5f                   je     1190 <main+0xf0>
    1131:       4d 89 d0                mov    %r10,%r8
    1134:       48 83 f9 07             cmp    $0x7,%rcx
    1138:       74 76                   je     11b0 <main+0x110>
    113a:       48 39 fa                cmp    %rdi,%rdx
    113d:       73 21                   jae    1160 <main+0xc0>
    113f:       0f b6 0a                movzbl (%rdx),%ecx
    1142:       48 89 d6                mov    %rdx,%rsi
    1145:       eb a1                   jmp    10e8 <main+0x48>

(-O3 is kind of disappointing; with -Os it fits in a single cache line.)

I'll add a comment explaining why it's there.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM but the commit message should be changed to something that’s a) descriptive of what this commit actually does and b) not a wordplay on a right-wing slogan before or during merging.

Copy link
Contributor

@ryanmurakami ryanmurakami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis The Moderation Team is requesting that you change the commit message and pull request title. The Moderation Team believes the similarities to an American politician's campaign slogan is not in line with the Code of Conduct's guide to use "welcoming and inclusive language". The Moderation Team is happy to provide external references should you need them.

@bnoordhuis bnoordhuis changed the title src: make InternalModuleReadJSON great again src: make InternalModuleReadJSON linear again Jan 14, 2020
Commits dcb6929, 4396beb and 8a96d05 turned the O(n) scan in
InternalModuleReadJSON() into an O(4n) scan. Fix the performance
regression by turning that into a linear scan again.
@bnoordhuis
Copy link
Member Author

not a wordplay on a right-wing slogan before or during merging.

A play on a left-wing slogan would've been okay then? I've changed it to something more boring, PTAL.

@Trott
Copy link
Member

Trott commented Jan 15, 2020

not a wordplay on a right-wing slogan before or during merging.

A play on a left-wing slogan would've been okay then?

The phrase is one that provokes strong reactions. I think it is reasonable to request that it be changed in a situation like this where using the phrase is unnecessary.

But yes, the fact that the slogan is right-wing is not really the problem, per se. Had it been a play on "Read my lips: no new taxes", I don't think it would generate the same reaction.

FWIW, I can definitely think of a few left-wing slogans that would be problematic.

I've changed it to something more boring, PTAL.

Let's make it even more boring and remove all vestiges of the wordplay. src: fix performance regression in node_file.cc or maybe src: fix performance regression in InternalModuleReadJSON?

Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with updated commit message

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

Trott pushed a commit that referenced this pull request Jan 15, 2020
Commits dcb6929, 4396beb and 8a96d05 turned the O(n) scan in
InternalModuleReadJSON() into an O(4n) scan. Fix the performance
regression by turning that into a linear scan again.

PR-URL: #31343
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@Trott
Copy link
Member

Trott commented Jan 15, 2020

Landed in 6623481.

I updated the commit message on landing.

@Trott Trott closed this Jan 15, 2020
@ljharb ljharb changed the title src: make InternalModuleReadJSON linear again src: fix performance regression in node_file.cc Jan 15, 2020
MylesBorins pushed a commit that referenced this pull request Jan 16, 2020
Commits dcb6929, 4396beb and 8a96d05 turned the O(n) scan in
InternalModuleReadJSON() into an O(4n) scan. Fix the performance
regression by turning that into a linear scan again.

PR-URL: #31343
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@codebytere codebytere mentioned this pull request Jan 16, 2020
codebytere pushed a commit that referenced this pull request Mar 14, 2020
Commits dcb6929, 4396beb and 8a96d05 turned the O(n) scan in
InternalModuleReadJSON() into an O(4n) scan. Fix the performance
regression by turning that into a linear scan again.

PR-URL: #31343
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
Commits dcb6929, 4396beb and 8a96d05 turned the O(n) scan in
InternalModuleReadJSON() into an O(4n) scan. Fix the performance
regression by turning that into a linear scan again.

PR-URL: #31343
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants