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

Allow type sharing for Object.create objects #3901

Merged
merged 1 commit into from
Oct 9, 2017

Conversation

MikeHolman
Copy link
Contributor

This is similar to my last iteration (#3834):
Use the normal Object type if prototype is Object.prototype
Use a new shared type for the case of null prototype (saved on javascriptLibrary)
Difference is instead of using my new cache for other types, use the existing machinery in CreateObjectType

@obastemur
Copy link
Collaborator

Do you think these changes will be obsolete? #3847

@MikeHolman
Copy link
Contributor Author

No, I don't really think these changes will interact. It seems like in your case comparing against null is slow, but for me it's when you have a null prototype things are slow because of type sharing issues.

@obastemur
Copy link
Collaborator

Let me re-phrase my question then.

What I understood from the PR -> Use a cached type ref. in case of requestedInlineSlotCapacity == 0 and prototype is null /or/ object conditions are met. Yes ?

So;
first question is out of curiosity; why do we need two types doing the same thing?

and second.. This PR introduces nullPrototypeObjectType that will be something to take care of while implementing #3847 (a.k.a Context independent, shared null && undefined) ? Could you please share more about your plans with this PR ?

@MikeHolman
Copy link
Contributor Author

The reason why I restrict to requestedInlineSlotCapacity == 0 is to restrict my change to impact only objects created with Object.create. I don't know exactly why the existing code creates new types instead of using objectTypes[requestedInlineSlotCapacity]. It appears that it is because this precludes shrinking the inline slot capacity, but I don't really get why that is useful. There is a lot of code dealing with shrinking inline slot capacity, so my base assumption has to be that it serves some function. And since this isn't related to the problem I'm trying to solve, I didn't want to touch it.

One of the reasons null needs its own special case is that existing infrastructure relies on internal properties on the prototype object to help share the type. However, null is a plain old RecyclableObject rather than a DynamicObject, and as such it doesn't have any internal properties.

I'm still not getting the connection with #3847. What do you think you will need to do to accommodate this change? As long as null is still a RecylableObject, I'm not doing anything fundamentally new. A user can already set their prototype to null (and SetPrototype takes a RecylableObject*), I'm just caching the type in JS library so different objects with null prototype can share a DynamicType.

Copy link
Collaborator

@ricobbe ricobbe left a comment

Choose a reason for hiding this comment

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

LGTM, although I don't have enough context to have high confidence in my evaluation.

@obastemur
Copy link
Collaborator

LGTM too. My questions were mostly informal.

@chakrabot chakrabot merged commit 874b0a6 into chakra-core:master Oct 9, 2017
chakrabot pushed a commit that referenced this pull request Oct 9, 2017
Merge pull request #3901 from MikeHolman:objcreateopt

This is similar to my last iteration (#3834):
Use the normal Object type if prototype is Object.prototype
Use a new shared type for the case of null prototype (saved on javascriptLibrary)
Difference is instead of using my new cache for other types, use the existing machinery in CreateObjectType
@rajatd
Copy link
Contributor

rajatd commented Oct 10, 2017

Look(s/ed) good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants