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: simplify native_immediate_callbacks #19339

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

  • Use std::list instead of std::vector because we do not need
    random access.
  • Use list.erase instead of moving elements then resizing to
    remove the first n elements in the list for simplicity and
    avoiding reallocation.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

- Use std::list instead of std::vector because we do not need
  random access.
- Use list.erase instead of moving elements then resizing to
  remove the first n elements in the list for simplicity and
  avoiding reallocation.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 14, 2018
@joyeecheung
Copy link
Member Author

@JacksonTian JacksonTian self-requested a review March 14, 2018 07:46
@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 14, 2018
@addaleax
Copy link
Member

Did you benchmark this? Using a std::list is usually slower and almost always has more memory overhead than std::vector if the elements are small… I did consider using a std::list for this, but chose not to because of that.

@joyeecheung
Copy link
Member Author

@addaleax I didn't. Should I use the HTTP2 benchmark (looks like that's where the native immediates are extensively used) or create an addon benchmark like bencharmk/misc/function_call?

@addaleax
Copy link
Member

Hm … an addon benchmark sounds like a lot of work for something like this, but I’m not sure that any particular subsystem uses this enough to reflect significant changes.

(which might also mean that it probably doesn’t quite matter that much anyway… 😄 )

@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 15, 2018

Started a HTTP2 benchmark: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/140

@addaleax
Copy link
Member

@joyeecheung It seems hard to write an addon benchmark, because getting addon and core ABI align is tricky due to all the flags we have, and because this patch itself switches the ABI… here’s what I’ve come up with by #defineing the right flags manually: https://gist.github.com/addaleax/95df2705479cf7c7cc2f3714ee4b36f9

I’m getting about 44 Mops/sec with the current implementation, and about 19.5 Mops/sec with this PR.

I’d leave it to you to make the call to say whether this is worth it, given that it doesn’t seem to affect the actual application code all that much.

But generally, if you have ideas for how to actually get that benchmark (or one like it) into core, that would be cool. 😄

@joyeecheung
Copy link
Member Author

@addaleax I'll close because I don't think it's worth the cost, thanks for writing up the benchmark!

I've attempted to run node-gyp from within the benchmark script before but failed to make it play with child_process (refs: #16934 ), probably can figure it out sometime later

@tniessen tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 24, 2018
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++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants