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: migrate to new V8 array API #24613

Closed
wants to merge 3 commits into from

Conversation

kt3k
Copy link
Contributor

@kt3k kt3k commented Nov 24, 2018

This PR uses new V8 API for array creation, instead of old one.

related PR: #24125

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

This change migrates the deprecated V8 Array API to new APIs.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. util Issues and PRs related to the built-in util module. labels Nov 24, 2018
@addaleax addaleax added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 24, 2018
@addaleax
Copy link
Member

src/node_util.cc Outdated

int state = promise->State();
ret->Set(env->context(), 0, Integer::New(isolate, state)).FromJust();
std::vector<Local<Value>> values;
Copy link
Member

Choose a reason for hiding this comment

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

values(2), as we can have a maximum of two values.

Copy link
Contributor

@refack refack Nov 25, 2018

Choose a reason for hiding this comment

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

Then way not simply:

Suggested change
std::vector<Local<Value>> values;
Local<Value> values[2] = { Integer::New(isolate, state) };
size_t number_of_values = 1;
if (state != Promise::PromiseState::kPending)
values[number_of_values++] = promise->Result();
DCHECK_LE(number_of_values, arraysize(values));
Local<Array> ret = Array::New(isolate, values, number_of_values);

Copy link
Member

Choose a reason for hiding this comment

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

That’s much longer and doesn’t really provide too much of an advantage over using a vector.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT it's only one line longer, and we get everything on the stack with no free-store allocations.
I thought that when possible, we prefer static sized data structures?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried @refack's suggestion, but I couldn't find a reasonable way to include DCHECK_LE in this file. DCHECK_LE is defined in deps/v8/src/base/logging.h, and when I add #include "../deps/v8/src/base/logging.h" at the top, the compile doesn't seem passing (relative path inside logging.h doesn't seem resolving...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed @TimothyGu's comment for the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With values(2), the values are initialized with empty values and it has size 2. So I needed to add an else block and .pop_back() the last (empty) value. (I think that's unnecessarily complex and not desirable code.)

So I changed the code to follow @refack's comment (without DCHECK_LE assertion).

This addresses the comment in nodejs#24613
@kt3k kt3k force-pushed the feature/node-util-improvement branch 2 times, most recently from fe987df to 0098eb2 Compare November 28, 2018 04:57
Use static sized data structure
@kt3k kt3k force-pushed the feature/node-util-improvement branch from 0098eb2 to 3729328 Compare November 28, 2018 12:50
@kt3k
Copy link
Contributor Author

kt3k commented Nov 28, 2018

(I force-pushed with empty change a few times to fix CI status but it keeps failing because of the rate limit of github API, which is used for fetching the commit message.)

@joyeecheung
Copy link
Member

@kt3k We don't have our own DCHECK macros - it's being added in #24359

@gireeshpunathil
Copy link
Member

@Trott
Copy link
Member

Trott commented Nov 29, 2018

Landed in 27139fc

@Trott Trott closed this Nov 29, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 29, 2018
This change migrates the deprecated V8 Array API to new APIs.

PR-URL: nodejs#24613
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
@kt3k
Copy link
Contributor Author

kt3k commented Nov 29, 2018

Thank you for the reviews! Thank you for landing!

@kt3k kt3k deleted the feature/node-util-improvement branch November 29, 2018 03:21
targos pushed a commit that referenced this pull request Nov 29, 2018
This change migrates the deprecated V8 Array API to new APIs.

PR-URL: #24613
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
This change migrates the deprecated V8 Array API to new APIs.

PR-URL: nodejs#24613
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
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++. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.