[runtime] Closure Creation from SetPrototypeProperties should be deferred to first call [v8/v8 : main]
Raphael Herouart (Gerrit)
Message from Raphael Herouart![]( "Open in Gerrit")
Set Ready For Review
Related details
- Raphael Herouart
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko added 17 comments![]( "Open in Gerrit")
BIND(&if_smi);Could you please introduce a helper function for unwrapping SFIs?
I'd be fine with landing this unwrapping CL behind a build flag just to be able to move forward. Otherwise I'm afraid doing this for EVERY load will be too expensive (note that this code is used for loading properties from regular objects too) and you'd have to deal with tons of JS2/SP3 regressions once you land it.
Actually, could you please run JS2/SP3 pinpoint jobs on this CL?
I think a better way would be to introduce a new field representation for not yet instantiated functions and call runtime to instantiate the function and update the representation in the map once we load from it. See how we deal with double representations in these functions.
Note, that we don't even need to bailout from the lookup - we already did the major part of the job, we know the holder, the property/dictionary index, etc - we just need to instantiate the function, update the map/object and return it.
TryCast<PrototypeSharedClosureInfo>(Why is this cast necessary given that the getter returns the right type?
if (has_closure_infos) {We can check `!closure_info.is_null()` instead.
prototype_shared_closure_info();This should be `prototype_info()` because its return type covers all possible types, while `prototype_shared_closure_info()` will crash if there will be something else (for example, Smi).
if (is_prototype_map()) {Nit: it would be nicer to avoid a lot of block nesting and just return if it's not a prototype map.
TaggedField<PrototypeSharedClosureInfo,I'm afraid that this field load might fail to perform the cast if the field contains something else. Given that there's no simple way of checking `has_prototype_closure_info()` we should always use `TryGetPrototypeSharedClosureInfo()`.
Please consider extending the type for `prototype_info` getter with `PrototypeSharedClosureInfo`.
TryCast<PrototypeSharedClosureInfo>(maybe_proto_shared_closure_info,Ditto: why is this cast necessary?
DirectHandle<Object> receiver_obj = it->GetHolder<Object>();Receiver is not correct - it's holder (this read could have been triggered via some random receiver containing our prototype in the prototype chain).
And with given iterator state it's guaranteed to be `JSObject`:
```
DirectHandle<JSObject> holder = it->GetHolder<JSObject>();
```
Please extract this SFI handling to a helper function.
*Runtime::SetObjectProperty(
it->isolate(), Cast<JSAny>(receiver), it->GetName(), val,
StoreOrigin::kNamed)
.ToHandleChecked(),This is an overkill. Ideally it would be nice if we could reuse the existing lookup iterator because it's already known everything about the property we've just loaded - we just need a version of `Object::SetDataProperty` (which is called at the bottom of `Runtime::SetObjectProperty(..)`) that does the store to the holder instead of receiver.
At least you can try this:
```
LookupIterator it2(isolate, holder, it->GetName(), LookupIterator::OWN_SKIP_INTERCEPTOR);
DCHECK_EQ(it2.state(), LookupIterator::DATA);
bool result = Object::SetProperty(&it2, val, StoreOrigin::kNamed).ToChecked();
CHECK(result); // This store must not throw and must not fail.
return val;
```
// there should be a closure info for it`.` at the end.
// Context in which closure(s) where definedPlease end comments with `.`.
prototype_shared_closure_info: PrototypeSharedClosureInfo|Undefined;I wonder, how common is the case when we'd have just a `PrototypeSharedClosureInfo` object? I mean it'd be way simpler to just add context/feedback cell array here instead of dealing with all the transitions between different modes.
feedeback_slot: uint16;Here and elsewhere: typo.
shared->set_feedeback_slot(current_slot);We need a fallback mechanism against overflowing the `feedback_slot` field.
Related details
- Raphael Herouart
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko added 1 comment![]( "Open in Gerrit")
Tagged<Context> ctx, Tagged<ClosureFeedbackCellArray> feedback_array) {These must be passed here as handles, otherwise if GC happens during allocation of a new object then these raw pointers might become stale.
Related details
- Raphael Herouart
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/111a6502310000
Related details
- Raphael Herouart
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/130b7b6a310000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/10632e86310000
Raphael Herouart (Gerrit)
Raphael Herouart added 1 comment![]( "Open in Gerrit")
prototype_shared_closure_info: PrototypeSharedClosureInfo|Undefined;I wonder, how common is the case when we'd have just a `PrototypeSharedClosureInfo` object? I mean it'd be way simpler to just add context/feedback cell array here instead of dealing with all the transitions between different modes.
@ish...@chromium.org You mean having the PrototypeSharedClosureInfo stored in the PrototypeInfo slot?
Likely the most common case. At setup time the prototype_info does not exist, object is in slow mode. It is actually made fast at first access.
In most of my unittests this is the case I am in.
Related details
- Igor Sheludko
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko added 1 comment![]( "Open in Gerrit")
prototype_shared_closure_info: PrototypeSharedClosureInfo|Undefined;Raphael HerouartI wonder, how common is the case when we'd have just a `PrototypeSharedClosureInfo` object? I mean it'd be way simpler to just add context/feedback cell array here instead of dealing with all the transitions between different modes.
@ish...@chromium.org You mean having the PrototypeSharedClosureInfo stored in the PrototypeInfo slot?
Likely the most common case. At setup time the prototype_info does not exist, object is in slow mode. It is actually made fast at first access.
In most of my unittests this is the case I am in.
Ah, right. If those constructors are not going to be called then it's not necessary to create PrototypeInfo objects.
Related details
- Raphael Herouart
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Raphael Herouart (Gerrit)
Raphael Herouart added 2 comments![]( "Open in Gerrit")
Tagged<Context> ctx, Tagged<ClosureFeedbackCellArray> feedback_array) {These must be passed here as handles, otherwise if GC happens during allocation of a new object then these raw pointers might become stale.
- Igor Sheludko
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Raphael Herouart (Gerrit)
Raphael Herouart added 10 comments![]( "Open in Gerrit")
Why is this cast necessary given that the getter returns the right type?
Done
if (has_closure_infos) {Raphael HerouartLeftover.
Done
We can check `!closure_info.is_null()` instead.
Done
This should be `prototype_info()` because its return type covers all possible types, while `prototype_shared_closure_info()` will crash if there will be something else (for example, Smi).
Done
Nit: it would be nicer to avoid a lot of block nesting and just return if it's not a prototype map.
Done
TryCast<PrototypeSharedClosureInfo>(maybe_proto_shared_closure_info,Raphael HerouartDitto: why is this cast necessary?
Done
DirectHandle<Object> receiver_obj = it->GetHolder<Object>();Receiver is not correct - it's holder (this read could have been triggered via some random receiver containing our prototype in the prototype chain).
And with given iterator state it's guaranteed to be `JSObject`:
```
DirectHandle<JSObject> holder = it->GetHolder<JSObject>();
```
Raphael HerouartPlease extract this SFI handling to a helper function.
Done
// there should be a closure info for itRaphael Herouart`.` at the end.
Done
Please end comments with `.`.
Done
feedeback_slot: uint16;Raphael HerouartHere and elsewhere: typo.
Done
Related details
- Igor Sheludko
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Raphael Herouart (Gerrit)
Raphael Herouart added 3 comments![]( "Open in Gerrit")
TaggedField<PrototypeSharedClosureInfo,Raphael HerouartI'm afraid that this field load might fail to perform the cast if the field contains something else. Given that there's no simple way of checking `has_prototype_closure_info()` we should always use `TryGetPrototypeSharedClosureInfo()`.
Please consider extending the type for `prototype_info` getter with `PrototypeSharedClosureInfo`.
Done
*Runtime::SetObjectProperty(
it->isolate(), Cast<JSAny>(receiver), it->GetName(), val,
StoreOrigin::kNamed)
.ToHandleChecked(),This is an overkill. Ideally it would be nice if we could reuse the existing lookup iterator because it's already known everything about the property we've just loaded - we just need a version of `Object::SetDataProperty` (which is called at the bottom of `Runtime::SetObjectProperty(..)`) that does the store to the holder instead of receiver.
At least you can try this:
```
LookupIterator it2(isolate, holder, it->GetName(), LookupIterator::OWN_SKIP_INTERCEPTOR);
DCHECK_EQ(it2.state(), LookupIterator::DATA);
bool result = Object::SetProperty(&it2, val, StoreOrigin::kNamed).ToChecked();
CHECK(result); // This store must not throw and must not fail.
return val;
```
Done
shared->set_feedeback_slot(current_slot);Raphael HerouartWe need a fallback mechanism against overflowing the `feedback_slot` field.
Done
Related details
- Igor Sheludko
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1284e916310000
Related details
- Igor Sheludko
- Raphael Herouart
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/168999c0b10000
Related details
- Igor Sheludko
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1463e1cd310000
Raphael Herouart (Gerrit)
Raphael Herouart added 1 comment![]( "Open in Gerrit")
BIND(&if_smi);Could you please introduce a helper function for unwrapping SFIs?
I'd be fine with landing this unwrapping CL behind a build flag just to be able to move forward. Otherwise I'm afraid doing this for EVERY load will be too expensive (note that this code is used for loading properties from regular objects too) and you'd have to deal with tons of JS2/SP3 regressions once you land it.
Actually, could you please run JS2/SP3 pinpoint jobs on this CL?
I think a better way would be to introduce a new field representation for not yet instantiated functions and call runtime to instantiate the function and update the representation in the map once we load from it. See how we deal with double representations in these functions.
Note, that we don't even need to bailout from the lookup - we already did the major part of the job, we know the holder, the property/dictionary index, etc - we just need to instantiate the function, update the map/object and return it.
- As for helper functions.
Done.
- As for Perf.
https://pinpoint-dot-chromeperf.appspot.com/job/1463e1cd310000
https://pinpoint-dot-chromeperf.appspot.com/job/168999c0b10000
I cannot see anything particularly bad and yet there is still some debug code to remove from this CL.
- As for the 2 last paragraphs of your comment
I am not sure understand 😊 "introduce a new field representation for not yet instantiated functions", but the main point of this optimization is to save some memory. By storing SFI rather than JS functions until needed.
NOTE: I'll safeguard all this behind a v8_flag so we can move forward.
Raphael Herouart (Gerrit)
Raphael Herouart added 1 comment![]( "Open in Gerrit")
BIND(&if_smi);Raphael HerouartCould you please introduce a helper function for unwrapping SFIs?
I'd be fine with landing this unwrapping CL behind a build flag just to be able to move forward. Otherwise I'm afraid doing this for EVERY load will be too expensive (note that this code is used for loading properties from regular objects too) and you'd have to deal with tons of JS2/SP3 regressions once you land it.
Actually, could you please run JS2/SP3 pinpoint jobs on this CL?
I think a better way would be to introduce a new field representation for not yet instantiated functions and call runtime to instantiate the function and update the representation in the map once we load from it. See how we deal with double representations in these functions.
Note, that we don't even need to bailout from the lookup - we already did the major part of the job, we know the holder, the property/dictionary index, etc - we just need to instantiate the function, update the map/object and return it.
- As for helper functions.
Done.
- As for Perf.https://pinpoint-dot-chromeperf.appspot.com/job/1463e1cd310000
https://pinpoint-dot-chromeperf.appspot.com/job/168999c0b10000I cannot see anything particularly bad and yet there is still some debug code to remove from this CL.
- As for the 2 last paragraphs of your commentI am not sure understand 😊 "introduce a new field representation for not yet instantiated functions", but the main point of this optimization is to save some memory. By storing SFI rather than JS functions until needed.
NOTE: I'll safeguard all this behind a v8_flag so we can move forward.
Done
Related details
- Igor Sheludko
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Raphael Herouart (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Igor Sheludko
- Leszek Swirski
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Raphael Herouart (Gerrit)
Message from Raphael Herouart![]( "Open in Gerrit")
- Igor Sheludko
- Leszek Swirski
- Raphael Herouart
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 2 comments![]( "Open in Gerrit")
DEFINE_WEAK_IMPLICATION(future, proto_assign_seq_lazy_func_opt)let's not do this yet, `future` is for things closer to shipping. Consider instead an implication from `experimental_fuzzing`
DEFINE_BOOL(proto_assign_seq_lazy_func_opt, false,`DEFINE_EXPERIMENTAL_FEATURE(proto_assign_seq_lazy_func_opt,`, otherwise this becomes immediately open to VRP
Related details
- Igor Sheludko
- Raphael Herouart
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Raphael Herouart (Gerrit)
Raphael Herouart added 2 comments![]( "Open in Gerrit")
DEFINE_WEAK_IMPLICATION(future, proto_assign_seq_lazy_func_opt)let's not do this yet, `future` is for things closer to shipping. Consider instead an implication from `experimental_fuzzing`
Done
`DEFINE_EXPERIMENTAL_FEATURE(proto_assign_seq_lazy_func_opt,`, otherwise this becomes immediately open to VRP
- Igor Sheludko
- Leszek Swirski
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko added 20 comments![]( "Open in Gerrit")
mostly nits modulo feedback_slot overflow
// Whether we are using SetPrototypeProperties together with LazyClosures```
// , basically, the value of --proto_assign_seq_lazy_func_opt flag to be
// used from builtins.
```
(Please finish comments with `.`).
DirectHandle<Context> ctx,Nit here and below: please use full words unless the name is too long (see the code around). So, `context`.
Label* if_not_lazy_closure, Label* slow, TNode<Object> value) {Please consider defining a type `JSAnyOrSharedFunctionInfo`, this would make it clear that this function is applicable for lazy closures case.
Return(value);Using `Return` in the helper limits its usefulness. Did you mean to jump to
`if_not_lazy_closure`?
Please either update the name of the helper or consider renaming it to
`GotoIfLazyClosure(value, if_lazy_closure)` and letting it fall through if it's not the case or to `BranchIfLazyClosure(value, if_blah, if_not_blah)` (see other GotoIf/BranchIf in CSA, the labels are the last arguments).
closure_info = DirectHandle<PrototypeSharedClosureInfo>(Nit: just `direct_handle(`.
if (TryCast<SharedFunctionInfo>(value, &shared)) {Please consider extracting the body of this `if` to a helper function to simplify the code here.
Tagged<Map> proto_map = holder->map();Nit: good practice is not to leave raw values around in function that can allocate even though you are not using them after allocation. Please replace the two usages with just `holder->map()`.
Tagged<Context> closure_ctx = closure_info->context();Same here: create the handle right away.
return it->GetDataValue();This method is not cheap, this should have been just `return value;`.
How about restructuring the code like this:
```
DirectHandle<Object> value = it->GetDataValue();
if (!flag) {
return value;
}
if (lazy_closure_case) {
...
}
return value;
```
class BodyDescriptor;Nit: for "Structs" (i.e. all tagged fields objects of fixed size) you can just use `using BodyDescriptor = StructBodyDescriptor;`.
Please add empty lines before and after this line.
// of closures`.` at the end.
feedback_slot: uint16;Please add a comment about this field.
static DirectHandle<Object> InstantiateIfSharedFunctionInfo(Nit: Chromium C++ style guide recommends putting such functions into anonymous namespaces instead of using `static` (see, there's one above).
if (!v8_flags.proto_assign_seq_lazy_func_opt) {I think the code in this function will be nicer if you return early if the value is not a `SharedFunctionInfo` (there will be less nesting).
auto closure_ctx = closure_info->context();This will be a raw `Tagged<Context>` value in a function that can allocate. Please "inline" it into the use place instead.
// set up. We can only take the lazy closure path it the contextTypo: `if`.
DCHECK(base::IsInRange(current_slot, 0, kMaxUInt16));Is there a limit somewhere that guarantees this assumption?
DCHECK(base::IsInRange(current_slot, 0, kMaxUInt16));Ditto.
// assert_test_function_fast_path(test_function_fast_path());Are these lines still causing issues?
Related details
- Leszek Swirski
- Raphael Herouart
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Raphael Herouart (Gerrit)
Raphael Herouart added 1 comment![]( "Open in Gerrit")
// assert_test_function_fast_path(test_function_fast_path());Are these lines still causing issues?
- Igor Sheludko
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Raphael Herouart (Gerrit)
Raphael Herouart added 17 comments![]( "Open in Gerrit")
// Whether we are using SetPrototypeProperties together with LazyClosures```
// , basically, the value of --proto_assign_seq_lazy_func_opt flag to be
// used from builtins.
```(Please finish comments with `.`).
Done
Nit here and below: please use full words unless the name is too long (see the code around). So, `context`.
Done
Label* if_not_lazy_closure, Label* slow, TNode<Object> value) {Please consider defining a type `JSAnyOrSharedFunctionInfo`, this would make it clear that this function is applicable for lazy closures case.
Done
Using `Return` in the helper limits its usefulness. Did you mean to jump to
`if_not_lazy_closure`?Please either update the name of the helper or consider renaming it to
`GotoIfLazyClosure(value, if_lazy_closure)` and letting it fall through if it's not the case or to `BranchIfLazyClosure(value, if_blah, if_not_blah)` (see other GotoIf/BranchIf in CSA, the labels are the last arguments).
Done
closure_info = DirectHandle<PrototypeSharedClosureInfo>(Raphael HerouartNit: just `direct_handle(`.
Done
Please consider extracting the body of this `if` to a helper function to simplify the code here.
Done
Nit: good practice is not to leave raw values around in function that can allocate even though you are not using them after allocation. Please replace the two usages with just `holder->map()`.
Done
Tagged<Context> closure_ctx = closure_info->context();Same here: create the handle right away.
Done
This method is not cheap, this should have been just `return value;`.
How about restructuring the code like this:
```
DirectHandle<Object> value = it->GetDataValue();
if (!flag) {
return value;
}
if (lazy_closure_case) {
...
}
return value;
```
Raphael Herouart`.` at the end.
Done
Please add a comment about this field.
Done
static DirectHandle<Object> InstantiateIfSharedFunctionInfo(Nit: Chromium C++ style guide recommends putting such functions into anonymous namespaces instead of using `static` (see, there's one above).
Done
I think the code in this function will be nicer if you return early if the value is not a `SharedFunctionInfo` (there will be less nesting).
Done
This will be a raw `Tagged<Context>` value in a function that can allocate. Please "inline" it into the use place instead.
Done
// set up. We can only take the lazy closure path it the contextRaphael HerouartTypo: `if`.
Done
DCHECK(base::IsInRange(current_slot, 0, kMaxUInt16));Is there a limit somewhere that guarantees this assumption?
Done
DCHECK(base::IsInRange(current_slot, 0, kMaxUInt16));Raphael HerouartDitto.
Done
Related details
- Igor Sheludko
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Raphael Herouart (Gerrit)
Raphael Herouart added 1 comment![]( "Open in Gerrit")
class BodyDescriptor;Nit: for "Structs" (i.e. all tagged fields objects of fixed size) you can just use `using BodyDescriptor = StructBodyDescriptor;`.
Please add empty lines before and after this line.
Done
Related details
- Igor Sheludko
- Leszek Swirski
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko voted and added 3 comments![]( "Open in Gerrit")
Votes added by Igor Sheludko
| Code-Review | +1 |
3 comments
TNode<JSAnyOrSharedFunctionInfo> value);Nit: please put the value argument first to be consistent with the other similar functions in CSA.
DirectHandle<SharedFunctionInfo> shared;
if (!TryCast<SharedFunctionInfo>(value, &shared)) {
return value;
}This part is common for both `proto_assign_seq_lazy_func_opt` and `!proto_assign_seq_lazy_func_opt` cases. How about moving the above if here?
Related details
- Leszek Swirski
- Raphael Herouart
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Raphael Herouart (Gerrit)
Raphael Herouart added 2 comments![]( "Open in Gerrit")
Nit: please put the value argument first to be consistent with the other similar functions in CSA.
Done
DirectHandle<SharedFunctionInfo> shared;
if (!TryCast<SharedFunctionInfo>(value, &shared)) {
return value;
}This part is common for both `proto_assign_seq_lazy_func_opt` and `!proto_assign_seq_lazy_func_opt` cases. How about moving the above if here?
Done
Related details
- Leszek Swirski
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski voted Code-Review+1![]( "Open in Gerrit")
- Raphael Herouart
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Raphael Herouart (Gerrit)
Raphael Herouart voted Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[runtime] Closure Creation from SetPrototypeProperties should be deferred to first call
This CL defers the creation of closures from `SetPrototypeProperties` to
when the function is first called.
When `SetPrototypeProperties` is used to assign multiple functions to a
prototype, instead of creating a `JSFunction` object for each one, this
change stores the `SharedFunctionInfo` directly on the object.
The `JSFunction` is only instantiated on the first access.This is a
performance optimization controlled by the new flag
`proto_assign_seq_lazy_func_opt`.
To support this lazy creation, a new `PrototypeSharedClosureInfo` struct
is introduced to store the necessary context for when the closure is
eventually created. The property access and IC mechanisms have been
updated to handle this deferred instantiation.
- M src/common/globals.h
- M src/diagnostics/objects-debug.cc
- M src/diagnostics/objects-printer.cc
- M src/execution/isolate-data-fields.h
- M src/execution/isolate-data.h
- M src/execution/isolate.cc
- M src/flags/flag-definitions.h
- M src/heap/factory-base.cc
- M src/heap/factory.cc
- M src/heap/factory.h
- M src/heap/heap-visitor.h
- M src/ic/accessor-assembler.cc
- M src/ic/accessor-assembler.h
- M src/objects/js-objects.cc
- M src/objects/map-inl.h
- M src/objects/map.cc
- M src/objects/map.h
- M src/objects/map.tq
- M src/objects/objects-definitions.h
- M src/objects/objects.cc
- M src/objects/objects.h
- M src/objects/prototype-info-inl.h
- M src/objects/prototype-info.h
- M src/objects/prototype-info.tq
- M src/objects/shared-function-info-inl.h
- M src/objects/shared-function-info.cc
- M src/objects/shared-function-info.h
- M src/objects/shared-function-info.tq
- M src/objects/transitions-inl.h
- M src/objects/transitions.cc
- M src/objects/transitions.h
- M src/roots/static-roots-intl-nowasm.h
- M src/roots/static-roots-intl-wasm.h
- M src/roots/static-roots-nointl-nowasm.h
- M src/roots/static-roots-nointl-wasm.h
- M src/runtime/runtime-literals.cc
Code-Review: +1 by Igor Sheludko, +1 by Leszek Swirski
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |