FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

timers: minor refinements to TimersList & stricter undefined/null checks by apapirovski · Pull Request #17429 · nodejs/node · GitHub

/ node Public

timers: minor refinements to TimersList & stricter undefined/null checks - #17429

Closed
apapirovski wants to merge 2 commits into
nodejs:masterfrom
apapirovski:patch-timers
ConversationCommits (2)ChecksFiles changed
Closed

timers: minor refinements to TimersList & stricter undefined/null checks#17429
apapirovski wants to merge 2 commits into
nodejs:masterfrom
apapirovski:patch-timers

Conversation

apapirovski commented Dec 2, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

A few small refinements to TimersList & ImmediateList:

  • Eliminate the createTimersList helper function and instead do all instantiation directly within the constructor
  • Define TIMEOUT_MAX as 2 ** 31 - 1 instead of having a comment explaining it
  • No longer use delete on unrefed & refed lists as it's expensive and offers no benefit since we never iterate over the dictionary object in regular usage, except in cases where user-code throws and we can likely accept the minute difference in performance in those cases
  • Stricter comparisons to null & undefined
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

timers

apapirovski added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Dec 2, 2017

apapirovski commented Dec 2, 2017

Copy link
Copy Markdown
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/11855/

mscdex commented Dec 2, 2017

Copy link
Copy Markdown
Contributor

Not using delete could leave a lot of entries in the various list objects. This is why we use delete in EventEmitter for example.

apapirovski commented Dec 2, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Not using delete could leave a lot of entries in the various list objects.

Yes but in practical terms this has basically no impact. I've tested access performance on an Object with 1e7 keys and 1e2 keys and it's constant. It's also likely that the keys will be pretty predictable since they're based on user code and unlikely to be random numbers, so constantly deleting and adding them is not really helping anyone.

This is why we use delete in EventEmitter for example.

And that is also being removed in #17324. As far as I can see, we're just sabotaging performance by using delete on Objects that are never iterated over and are likely to have a predictable list of properties.

apapirovski commented Dec 2, 2017

Copy link
Copy Markdown
Contributor Author

@mscdex Btw if there's a legitimate reason to not do this that I'm missing, I'm definitely all ears and will remove that change. I just haven't found it in my testing... And that also applies to the events PR linked above. Thanks for the feedback!

Trott commented Dec 2, 2017

Copy link
Copy Markdown
Member

Maybe run benchmark/timers on this?

apapirovski commented Dec 2, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@Trott yeah, it's running right now. Forgot to post a link.

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/69/console

Trott approved these changes Dec 2, 2017
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM if benchmarks and CI are OK.

lpinca commented Dec 3, 2017

Copy link
Copy Markdown
Member

@apapirovski is memory usage the same without delete?

mscdex commented Dec 3, 2017

Copy link
Copy Markdown
Contributor

@apapirovski My concern was not about performance, but memory usage.

IMHO it is not safe to make any assumptions about what gets passed to setTimeout(), etc. as far as the timeout value goes.

mscdex commented Dec 3, 2017

Copy link
Copy Markdown
Contributor

And that is also being removed in #17324

I did not realize that until now, that is a bad idea IMHO. Even third-party EventEmitter implementations that used to do this switched to delete because of memory issues.

apapirovski commented Dec 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I did not realize that until now, that is a bad idea IMHO. Even third-party EventEmitter implementations that used to do this switched to delete because of memory issues.

Addressed. I did actually look at memory as an issue but I think I had my iterations set too low. Once I bumped it up enough, I'm seeing regressions in performance too — not just memory.

That said, if our policy is that we don't want to assume end-user input then delete cannot be removed — I don't think the memory issue can ever possibly be optimized enough.

Same as events, I think I was too optimistic re: user input being constant (to an extent).

timers: improvements to TimersList management
5ecc4a9 
No longer use delete to remove keys on unrefed & refed lists,
instead simply set to undefined.

Move all the TimersList instantiation code into the constructor.

Compare values to undefined & null as appropriate instead of
truthy or falsy.
timers: compare to null not falsey
253341a

apapirovski commented Dec 7, 2017

Copy link
Copy Markdown
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/11940/

apapirovski commented Dec 7, 2017

Copy link
Copy Markdown
Contributor Author

@lpinca would you mind reviewing this given your prior comment here? The delete issue has been resolved. Thanks!

lpinca approved these changes Dec 7, 2017
View reviewed changes

lpinca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

SGTM.

apapirovski added a commit that referenced this pull request Dec 7, 2017
timers: improvements to TimersList management
e55b7d6 
Move all the TimersList instantiation code into the constructor.

Compare values to undefined & null as appropriate instead of
truthy or falsy.

PR-URL: #17429
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

apapirovski commented Dec 7, 2017

Copy link
Copy Markdown
Contributor Author

Landed in e55b7d6

apapirovski closed this Dec 7, 2017
apapirovski deleted the patch-timers branch December 7, 2017 22:07
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
timers: improvements to TimersList management
1fbc548 
Move all the TimersList instantiation code into the constructor.

Compare values to undefined & null as appropriate instead of
truthy or falsy.

PR-URL: #17429
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
timers: improvements to TimersList management
89d31ee 
Move all the TimersList instantiation code into the constructor.

Compare values to undefined & null as appropriate instead of
truthy or falsy.

PR-URL: #17429
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v8.x labels Dec 20, 2017
BethGriggs removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v8.x labels Sep 19, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes
lpinca lpinca approved these changes

Assignees

No one assigned

Labels

timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL