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: refactor of CopyProperties with V8 5.5 DefineProperty #11102

Closed
wants to merge 1 commit into from

Conversation

AnnaMag
Copy link
Member

@AnnaMag AnnaMag commented Feb 1, 2017

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

vm: remove JavaScript code from CopyProperties()

CopyProperties() is refactored to use the V8 5.5 DefineProperty() API call.

The change maintains current behavior. This is step 1
in removing the CopyProperties() function (follow-up PR).
API improvements fix V8 SetNamedPropertyHandler
to properly treat the ES6 syntax and CopyProperties()
becomes redundant.

Strings used as property attributes and accessors
are defined as persistent strings in src/env.h.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels Feb 1, 2017
@AnnaMag
Copy link
Member Author

AnnaMag commented Feb 1, 2017

cc/ @fhinkel

@AnnaMag AnnaMag changed the title refactor of CopyProperties in node_contextify.cc using V8 5.5 DefineProperty src: refactor of CopyProperties in node_contextify.cc using V8 5.5 DefineProperty Feb 1, 2017
->GetOwnPropertyDescriptor(context, key).ToLocalChecked());

bool isAccessor = object_from_vm_context
->Has(context, String::NewFromUtf8(env()->isolate(), "get"))
Copy link
Contributor

@mscdex mscdex Feb 1, 2017

Choose a reason for hiding this comment

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

I don't know what the norm/rule is offhand, but perhaps we should make these persistent strings (defined in src/env.h)?

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 correct. We should create pre-allocated string in env.h else this will allocate v8::String every time we call CopyProperties.

Copy link
Member

Choose a reason for hiding this comment

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

Generally, if the string is going to be used frequently or more than once. I'm definitely + 1 on making these persistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

changes included. Thanks!

@fhinkel
Copy link
Member

fhinkel commented Feb 1, 2017

Can you change to commit message so it matches our guidelines please? It should be at most 50 characters long.

->Get(context, String::NewFromUtf8(env()->isolate(), "writable"))
.ToLocalChecked())->BooleanValue();

desc = new PropertyDescriptor(value, writable);
Copy link
Member

Choose a reason for hiding this comment

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

Who manages the lifetime of desc? When will get cleaned up?

Copy link
Member

Choose a reason for hiding this comment

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

It's deleted at the end of the function.

Copy link
Member

Choose a reason for hiding this comment

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

ah, didn't notice that. Thanks @fhinkel

@AnnaMag AnnaMag force-pushed the v8-5.5_CP branch 4 times, most recently from ba5235e to 93ae019 Compare February 2, 2017 19:21
@AnnaMag AnnaMag changed the title src: refactor of CopyProperties in node_contextify.cc using V8 5.5 DefineProperty src: refactor of CopyProperties with V8 5.5 DefineProperty Feb 2, 2017
clone_property_method = Local<Function>::Cast(script->Run());
CHECK(clone_property_method->IsFunction());
Local<Object> desc_from_vm_context = Local<Object>::Cast(global
->GetOwnPropertyDescriptor(context, key).ToLocalChecked());
Copy link
Member

Choose a reason for hiding this comment

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

I think it's more common to use As<T>() than using Local<T>::Cast in the codebase, like

        Local<Object> desc_from_vm_context = global->GetOwnPropertyDescriptor(context, key)
            .ToLocalChecked().As<Object>();

Copy link
Member Author

Choose a reason for hiding this comment

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

True- I didn't realize this file was the only one in src/ using Local<T>::Cast .
Good point!

if (!isAccessor) {
Local<Value> value = Local<Value>::Cast(desc_from_vm_context
->Get(context, env()->value_string())
.ToLocalChecked());
Copy link
Member

Choose a reason for hiding this comment

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

Same here (Local<Value>), though in this case you don't even need to cast because Get() returns a MaybeLocal<Value>.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This line seems to prevent the file from compiling.

Copy link
Member Author

@AnnaMag AnnaMag Feb 2, 2017

Choose a reason for hiding this comment

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

Thanks @TimothyGu.
I have just re-compiled it and re-run the tests. All seems to be working fine.

@AnnaMag AnnaMag force-pushed the v8-5.5_CP branch 3 times, most recently from a525064 to 128aeb2 Compare February 3, 2017 11:08
global->GetOwnPropertyDescriptor(context, key)
.ToLocalChecked().As<Object>();

bool isAccessor = desc_from_vm_context
Copy link
Member

Choose a reason for hiding this comment

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

Style: can you name this is_accessor?

->Has(context, env()->get_string())
.FromJust() || desc_from_vm_context
->Has(context, env()->set_string())
.FromJust();
Copy link
Member

Choose a reason for hiding this comment

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

This is mostly just personal preference but it's kind of a run-on line of code that's arguably more legible when written like this:

bool is_accessor =
    desc_from_vm_context->Has(context, env()->get_string())
    .FromJust() ||
    desc_from_vm_context->Has(context, env()->set_string())
    .FromJust();

If you shorten desc_from_vm_context you can probably even make it fit on fewer lines.

But whatever you do, the convention in core is to break after the '=' when the RHS doesn't fit on a single line. Can you update that here and below?


bool writable = desc_from_vm_context
->Get(context, env()->writable_string()).ToLocalChecked()
.As<Value>()->BooleanValue();
Copy link
Member

Choose a reason for hiding this comment

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

You don't have to cast to Value.


PropertyDescriptor* desc;

if (!isAccessor) {
Copy link
Member

Choose a reason for hiding this comment

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

Minor style issue: don't negate in conditionals unless necessary. Remove the ! and swap the else/then clauses, it's easier to read.

.As<Value>()->BooleanValue());
desc->set_enumerable(desc_from_vm_context
->Get(context, env()->enumerable_string()).ToLocalChecked()
.As<Value>()->BooleanValue());
Copy link
Member

Choose a reason for hiding this comment

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

You can get rid of the desc = new PropertyDescriptor(...) and delete desc calls if you wrap this code in a lambda, like this:

auto configure = [&] (PropertyDescriptor* desc) {
  // ...
};

Which you then call like this:

if (is_accessor) {
  // ...
  PropertyDescriptor desc(get, set);
  configure(&desc);
} else {
  // ...
  PropertyDescriptor desc(value, writable);
  configure(&desc);
}

Copy link
Member Author

@AnnaMag AnnaMag Feb 3, 2017

Choose a reason for hiding this comment

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

I did not find a way to avoid doubling the DefineProperty call after configure(&desc):

sandbox_obj->DefineProperty(context, key, desc);

Is it ok?

@AnnaMag
Copy link
Member Author

AnnaMag commented Feb 3, 2017

@bnoordhuis, updated. Thanks!

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Almost there.


PropertyDescriptor desc(get, set);
configure(&desc);
sandbox_obj->DefineProperty(context, key, desc);
Copy link
Member

Choose a reason for hiding this comment

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

You can move the DefineProperty call into lambda. Invoke it with sandbox_obj->DefineProperty(context, key, *desc); (note the star.)

Copy link
Member

Choose a reason for hiding this comment

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

If we call sandbox->DefineProperty() inside configure, which should probably rename the lambda. Maybe define_property_on_sandbox()?

Copy link
Member Author

@AnnaMag AnnaMag Feb 4, 2017

Choose a reason for hiding this comment

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

I included the improvements and suggestions. Thanks!

.As<Value>()->BooleanValue());
desc->set_enumerable(desc_vm_context
->Get(context, env()->enumerable_string()).ToLocalChecked()
.As<Value>()->BooleanValue());
Copy link
Member

Choose a reason for hiding this comment

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

You don't need the casts to Value here, it's already a Value. Can you use the BooleanValue() overloads that take a v8::Context? (Below too.)

CopyProperties() is refactored to use
the V8 5.5 DefineProperty() API call.
The change does not alter current behaviour.
It is a step prior to removing the function
CopyProperties, which becomes reduntant
after fixes of V8 SetNamedPropertyHandler
in 5.5. V8.

Strings used as property attributes
(value, enumerable etc) and accessors
are defined as persistent strings
in src/env.h
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

->Get(context, env()->enumerable_string()).ToLocalChecked()
->BooleanValue(context).FromJust());
sandbox_obj->DefineProperty(context, key, *desc);
};
Copy link
Member

Choose a reason for hiding this comment

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

We normally indent code inside lambdas with two spaces, like regular functions. However, if the linter doesn't complain, I guess I shouldn't either.

@fhinkel
Copy link
Member

fhinkel commented Feb 5, 2017

Thanks! Landed in 67af1ad.

fhinkel pushed a commit to fhinkel/node that referenced this pull request Feb 5, 2017
CopyProperties() is refactored to use
the V8 5.5 DefineProperty() API call.
The change does not alter current behaviour.
It is a step prior to removing the function
CopyProperties, which becomes reduntant
after fixes of V8 SetNamedPropertyHandler
in 5.5. V8.

Strings used as property attributes
(value, enumerable etc) and accessors
are defined as persistent strings
in src/env.h

PR-URL: nodejs#11102
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@italoacasas
Copy link
Contributor

In order to land this on v7 we need to wait for the v8 5.5 backport #11029

italoacasas pushed a commit that referenced this pull request Feb 10, 2017
CopyProperties() is refactored to use
the V8 5.5 DefineProperty() API call.
The change does not alter current behaviour.
It is a step prior to removing the function
CopyProperties, which becomes reduntant
after fixes of V8 SetNamedPropertyHandler
in 5.5. V8.

Strings used as property attributes
(value, enumerable etc) and accessors
are defined as persistent strings
in src/env.h

PR-URL: #11102
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
CopyProperties() is refactored to use
the V8 5.5 DefineProperty() API call.
The change does not alter current behaviour.
It is a step prior to removing the function
CopyProperties, which becomes reduntant
after fixes of V8 SetNamedPropertyHandler
in 5.5. V8.

Strings used as property attributes
(value, enumerable etc) and accessors
are defined as persistent strings
in src/env.h

PR-URL: nodejs#11102
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
CopyProperties() is refactored to use
the V8 5.5 DefineProperty() API call.
The change does not alter current behaviour.
It is a step prior to removing the function
CopyProperties, which becomes reduntant
after fixes of V8 SetNamedPropertyHandler
in 5.5. V8.

Strings used as property attributes
(value, enumerable etc) and accessors
are defined as persistent strings
in src/env.h

PR-URL: nodejs#11102
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@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++. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants