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

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented Dec 2, 2017 •
edited
Loading

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 apapirovski added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Dec 2, 2017
@apapirovski
Copy link
Member Author

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

@mscdex
Copy link
Contributor

mscdex commented Dec 2, 2017

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
Copy link
Member Author

apapirovski commented Dec 2, 2017 •
edited
Loading

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
Copy link
Member 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
Copy link
Member

Trott commented Dec 2, 2017

Maybe run benchmark/timers on this?

@apapirovski
Copy link
Member Author

apapirovski commented Dec 2, 2017 •
edited
Loading

@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
Trott approved these changes Dec 2, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if benchmarks and CI are OK.

@lpinca
Copy link
Member

lpinca commented Dec 3, 2017

@apapirovski is memory usage the same without delete?

@mscdex
Copy link
Contributor

mscdex commented Dec 3, 2017

@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
Copy link
Contributor

mscdex commented Dec 3, 2017

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
Copy link
Member Author

apapirovski commented Dec 3, 2017 •
edited
Loading

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).

@apapirovski
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.
@apapirovski
timers: compare to null not falsey
253341a
@apapirovski
Copy link
Member Author

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

@apapirovski
Copy link
Member Author

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

lpinca
lpinca approved these changes Dec 7, 2017
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

SGTM.

apapirovski added a commit that referenced this pull request Dec 7, 2017
@apapirovski
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@apapirovski
Copy link
Member Author

Landed in e55b7d6

@apapirovski apapirovski closed this Dec 7, 2017
@apapirovski apapirovski deleted the patch-timers branch December 7, 2017 22:07
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@apapirovski @MylesBorins
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@apapirovski @MylesBorins
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
@gibfahn 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 BethGriggs removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v8.x labels Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott 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
@apapirovski @mscdex @Trott @lpinca @BethGriggs @gibfahn