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.

Sign up for GitHub

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

Jump to bottom

src: remove static variables from string_search #20541

Closed
wants to merge 1 commit into from
Closed

src: remove static variables from string_search #20541

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented May 5, 2018

These variables can as well be stack-allocated. This avoids
relying on global state that is not protected by mutexes.

Thanks to Stephen Belanger for reviewing this change in its original PR.

Refs: ayojs/ayo#82

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@addaleax addaleax added buffer Issues and PRs related to the buffer subsystem. worker Issues and PRs related to Worker support. labels May 5, 2018
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels May 5, 2018
@addaleax
Copy link
Member Author

addaleax commented May 5, 2018 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-commit/18247/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/179/

Edit: CI is green, and benchmark results seem okay to me.

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 5, 2018
bnoordhuis
bnoordhuis reviewed May 6, 2018
View reviewed changes
src/string_search.h Outdated
// Table used temporarily while building the BoyerMoore good suffix
// shift table.
static int kSuffixTable[kBMMaxShift + 1];
int kSuffixTable[kBMMaxShift + 1];
Copy link
Member

Choose a reason for hiding this comment

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

I'd rename them to suffix_table_, etc., and inline their uses (i.e., get rid of the getters); that makes the code a little safer because it won't drop the array boundaries like it does now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! We’re still doing weird things with the array boundaries because of the - start_ computation which I’m not totally understanding, to be honest, but I don’t see anything speaking against inlining these.

@addaleax
Copy link
Member Author

addaleax commented May 6, 2018

Fresh CI: https://ci.nodejs.org/job/node-test-commit/18268/

@addaleax
src: remove static variables from string_search
d0f65aa 
These variables can as well be stack-allocated. This avoids
relying on global state that is not protected by mutexes.

Thanks to Stephen Belanger for reviewing this change in its original PR.

Refs: ayojs/ayo#82
@addaleax addaleax force-pushed the no-static-stringsearch branch from dad3a7c to d0f65aa Compare May 9, 2018 16:30
@addaleax
Copy link
Member Author

addaleax commented May 9, 2018

Rebased. One more CI: https://ci.nodejs.org/job/node-test-commit/18338/

@addaleax
Copy link
Member Author

addaleax commented May 9, 2018

Landed in 945da6d

@addaleax addaleax closed this May 9, 2018
@addaleax addaleax deleted the no-static-stringsearch branch May 9, 2018 18:21
addaleax added a commit that referenced this pull request May 9, 2018
@addaleax
src: remove static variables from string_search
945da6d 
These variables can as well be stack-allocated. This avoids
relying on global state that is not protected by mutexes.

Thanks to Stephen Belanger for reviewing this change in its original PR.

Refs: ayojs/ayo#82

PR-URL: #20541
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request May 12, 2018
@addaleax @targos
src: remove static variables from string_search
43ec938 
These variables can as well be stack-allocated. This avoids
relying on global state that is not protected by mutexes.

Thanks to Stephen Belanger for reviewing this change in its original PR.

Refs: ayojs/ayo#82

PR-URL: #20541
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@addaleax addaleax mentioned this pull request May 14, 2018
Merged
addaleax added a commit that referenced this pull request May 14, 2018
@addaleax
2018-05-15, Version 10.2.0 (Current), @???
3b86468 
Notable Changes:

* **addons**:
  - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668)
* **assert**:
  - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485)
* **crypto**:
  - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039)
* **http**:
  - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611)
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (helloshuangzi) [#20639](#20639)
  - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377)
  - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541)
* **timers**:
  - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298)

PR-URL: #20724
addaleax added a commit that referenced this pull request May 15, 2018
@addaleax
2018-05-15, Version 10.2.0 (Current), @???
411ecf2 
Notable Changes:

* **addons**:
  - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668)
* **assert**:
  - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485)
* **crypto**:
  - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039)
* **http**:
  - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611)
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (helloshuangzi) [#20639](#20639)
  - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377)
  - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541)
* **esm**:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules. (Gus Caplan) [#20403](#20403)
* **timers**:
  - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298)

PR-URL: #20724
addaleax added a commit that referenced this pull request May 15, 2018
@addaleax
2018-05-15, Version 10.2.0 (Current), @???
d0eec7b 
Notable Changes:

* **addons**:
  - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668)
* **assert**:
  - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485)
* **crypto**:
  - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039)
* **http**:
  - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611)
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (Allen Yonghuang Wang) [#20639](#20639)
  - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377)
  - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541)
* **esm**:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules. (Gus Caplan) [#20403](#20403)
* **timers**:
  - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298)

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 22, 2018
@addaleax @MylesBorins
2018-05-23, Version 10.2.0 (Current)
d765fb6 
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 23, 2018
@addaleax @MylesBorins
2018-05-23, Version 10.2.0 (Current)
f6b6b3f 
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 23, 2018
@addaleax @MylesBorins
2018-05-23, Version 10.2.0 (Current)
5cbb905 
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 23, 2018
@addaleax @MylesBorins
2018-05-23, Version 10.2.0 (Current)
82f1811 
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
@snyk-bot snyk-bot mentioned this pull request Mar 18, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Apr 1, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@addaleax @bnoordhuis @jasnell @TimothyGu @cjihrig @nodejs-github-bot