Bug 194706 – Cache CompactVariableMap::Handle instead of VariableEnvironment for UnlinkedFunctionExecutable

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Tadeu Zagallo]

Similar to https://bugs.webkit.org/show_bug.cgi?id=194583, but for the bytecode cache.

[Tadeu Zagallo]

Created attachment 362118 [details]
Patch

[Saam Barati]

Comment on attachment 362118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362118&action=review

I think this is slightly off but I’m not 100% sure. Since Handle is responsible for bumping/decrementing the ref count, I’m not sure we properly model that, since we might decode only N handles, but have the ref count as M. I wonder if it’d make more sense to iteratively build the ref count as we decode handles?

> Source/JavaScriptCore/runtime/CachedTypes.cpp:941
> +        CompactVariableMap* map = new CompactVariableMap;

This looks like a leak to me. How is this supposed to be handled? Maybe this should return Ref, and this should be adoptRef?

> Source/JavaScriptCore/runtime/CachedTypes.cpp:962
> +        handle.m_map = m_map.decode(decoder);

I’m assuming the decoder already hash-conses stuff?

> Source/JavaScriptCore/runtime/CachedTypes.cpp:963
> +        return handle;

It worries me that we somehow don’t end up updating the map’s ref count in place. For example, when we encode a graph of code blocks, there may be Handle’s that live outside that graph. If so, we’re now leaking those VariableEnvironments since we’re encoding a global ref count. I think we should be smart enough to rebuild it local to the map we’re re-creating.

[Tadeu Zagallo]

Comment on attachment 362118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362118&action=review

>> Source/JavaScriptCore/runtime/CachedTypes.cpp:941
>> +        CompactVariableMap* map = new CompactVariableMap;
> 
> This looks like a leak to me. How is this supposed to be handled? Maybe this should return Ref, and this should be adoptRef?

This is how CachedPtr is supposed to work right now. CachedRefPtr has a CachedPtr, which it assume starts with a refCount of 1, since we must keep it alive during decoding. Each call to RefPtr decode increases the refCount by 1 and adoptRef's the pointer. When we destroy the Decoder, it derefs all the RefPtrs, to balance the initial refCount.

>> Source/JavaScriptCore/runtime/CachedTypes.cpp:962
>> +        handle.m_map = m_map.decode(decoder);
> 
> I’m assuming the decoder already hash-conses stuff?

Yes, it does.

>> Source/JavaScriptCore/runtime/CachedTypes.cpp:963
>> +        return handle;
> 
> It worries me that we somehow don’t end up updating the map’s ref count in place. For example, when we encode a graph of code blocks, there may be Handle’s that live outside that graph. If so, we’re now leaking those VariableEnvironments since we’re encoding a global ref count. I think we should be smart enough to rebuild it local to the map we’re re-creating.

I'm not sure I follow, but `m_map.decode` will increase the ref count.  We also don't encode the ref count per se, CachedRefPtr is just encoded as a pointer, and its only special behavior is rebuilding the ref count on decode.

[Saam Barati]

Comment on attachment 362118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362118&action=review

>>> Source/JavaScriptCore/runtime/CachedTypes.cpp:941
>>> +        CompactVariableMap* map = new CompactVariableMap;
>> 
>> This looks like a leak to me. How is this supposed to be handled? Maybe this should return Ref, and this should be adoptRef?
> 
> This is how CachedPtr is supposed to work right now. CachedRefPtr has a CachedPtr, which it assume starts with a refCount of 1, since we must keep it alive during decoding. Each call to RefPtr decode increases the refCount by 1 and adoptRef's the pointer. When we destroy the Decoder, it derefs all the RefPtrs, to balance the initial refCount.

Got it.

>>> Source/JavaScriptCore/runtime/CachedTypes.cpp:963
>>> +        return handle;
>> 
>> It worries me that we somehow don’t end up updating the map’s ref count in place. For example, when we encode a graph of code blocks, there may be Handle’s that live outside that graph. If so, we’re now leaking those VariableEnvironments since we’re encoding a global ref count. I think we should be smart enough to rebuild it local to the map we’re re-creating.
> 
> I'm not sure I follow, but `m_map.decode` will increase the ref count.  We also don't encode the ref count per se, CachedRefPtr is just encoded as a pointer, and its only special behavior is rebuilding the ref count on decode.

You're misunderstanding my comment here. It's worth just reading the implementation in to see what I'm saying below:
CompactVariableMap::get
and
Handle::~Handle

Basically, the reason we have a HashMap<CompactVariableMapKey, unsigned> instead of HashSet<CompactVariableMapKey> is that we use the value field in the HashMap as a ref count. Each handle is responsible for a +1 ref in that HashMap. When we encode this object, we may only encode a subset of Handles into the map. However, we're also encoding the original ref count of *all* handles. So when we decode the original ref count, we're now in a state where we may leak some CompactVariableEnvironments because our new map has fewer handles than the ref count suggests.

[Tadeu Zagallo]

Created attachment 362435 [details]
Patch

[Saam Barati]

Comment on attachment 362435 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362435&action=review

> Source/JavaScriptCore/runtime/CachedTypes.cpp:901
> +        CompactVariableEnvironment* env = new CompactVariableEnvironment;

I guess I'm still super confused about the lifetime of this object. Who is responsible for deleting this?

- Let's say we are the thing that adds this to the VM's map. The VM's environment map then deletes us when all handles go away? How do we end up being deleted? Basically, I'm worried if CachedPtr has some lifecycle tied to it, do we end up double freeing this object once all handles go away?
- Let's say something identical to us is already in the map, so all handles are actually wrapping the prior thing. Who deletes us?

> Source/JavaScriptCore/runtime/CachedTypes.cpp:922
> +        return decoder.vm().m_compactVariableMap->get(m_environment.decode(decoder), CompactVariableMap::DeleteUnusedEnvironment::No);

see comment above.

[Tadeu Zagallo]

Comment on attachment 362435 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362435&action=review

>> Source/JavaScriptCore/runtime/CachedTypes.cpp:901
>> +        CompactVariableEnvironment* env = new CompactVariableEnvironment;
> 
> I guess I'm still super confused about the lifetime of this object. Who is responsible for deleting this?
> 
> - Let's say we are the thing that adds this to the VM's map. The VM's environment map then deletes us when all handles go away? How do we end up being deleted? Basically, I'm worried if CachedPtr has some lifecycle tied to it, do we end up double freeing this object once all handles go away?
> - Let's say something identical to us is already in the map, so all handles are actually wrapping the prior thing. Who deletes us?

I think you're right, there's still a bug here.

- I think in this case, it's correct. CachedPtr never frees the pointer, so yes, if we added to the map the pointer will be deleted by ~Handle.
- Here is where I believe we still have the bug. If the "thing identical to us" have been placed in the map by this decoder, then it should be the same pointer, since we hash cons it. The case I did not consider was if the thing identical to us was placed in the map somewhere else.

I think the correct solution is that we need to know if the pointer was newly created or returned from cache and DeleteUnusedEnvironment should reflect it.

[Tadeu Zagallo]

Created attachment 362485 [details]
Patch

[Saam Barati]

Comment on attachment 362485 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362485&action=review

> Source/JavaScriptCore/runtime/CachedTypes.cpp:937
> +        CompactVariableEnvironment* environment = m_environment.decode(decoder, isNewAllocation);

This still doesn’t look right. Suppose we see the environment twice from two handles. Suppose that environment already exists in the VM too. And now we’re decoding the second one and we hash cons to the first one. The first one is now a destructed object.

Maybe what you originally had is closer to correct. I’m just confused whose job it is to free the environment at the end.

[Saam Barati]

Comment on attachment 362485 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362485&action=review

>> Source/JavaScriptCore/runtime/CachedTypes.cpp:937
>> +        CompactVariableEnvironment* environment = m_environment.decode(decoder, isNewAllocation);
> 
> This still doesn’t look right. Suppose we see the environment twice from two handles. Suppose that environment already exists in the VM too. And now we’re decoding the second one and we hash cons to the first one. The first one is now a destructed object.
> 
> Maybe what you originally had is closer to correct. I’m just confused whose job it is to free the environment at the end.

I meant to write this comment on the LOC below this one

[Tadeu Zagallo]

Created attachment 362498 [details]
Patch

[Tadeu Zagallo]

Comment on attachment 362485 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362485&action=review

>>> Source/JavaScriptCore/runtime/CachedTypes.cpp:937
>>> +        CompactVariableEnvironment* environment = m_environment.decode(decoder, isNewAllocation);
>> 
>> This still doesn’t look right. Suppose we see the environment twice from two handles. Suppose that environment already exists in the VM too. And now we’re decoding the second one and we hash cons to the first one. The first one is now a destructed object.
>> 
>> Maybe what you originally had is closer to correct. I’m just confused whose job it is to free the environment at the end.
> 
> I meant to write this comment on the LOC below this one

That does make sense. It seems that we have to control the lifecycle here, so I added yet another patch that does that. Thanks for all the reviews!

[Saam Barati]

Comment on attachment 362498 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362498&action=review

> Source/JavaScriptCore/runtime/CachedTypes.cpp:947
> +        return decoder.vm().m_compactVariableMap->get(environment, isNewEntry);
> +        if (isNewAllocation && !isNewEntry) {
> +            decoder.addFinalizer([=] {
> +                delete environment;
> +            });
> +        }

This can't be right ;-)

I vote for adding some tests at this point to ensure you get the behavior you want.

[Saam Barati]

Comment on attachment 362498 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362498&action=review

> Source/JavaScriptCore/runtime/CachedTypes.cpp:946
> +        if (isNewAllocation && !isNewEntry) {
> +            decoder.addFinalizer([=] {
> +                delete environment;
> +            });

Logically, this looks correct to me. But you should verify with testing once you remove the errant early return.

[Tadeu Zagallo]

Comment on attachment 362498 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362498&action=review

>> Source/JavaScriptCore/runtime/CachedTypes.cpp:947
>> +        }
> 
> This can't be right ;-)
> 
> I vote for adding some tests at this point to ensure you get the behavior you want.

damn, I can't believe I did this. I don't know why the compiler didn't complain about it either.

I agree that sounds reasonable, it was a lot of iterations. Should I make it an API test?

[Saam Barati]

Comment on attachment 362498 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362498&action=review

>>> Source/JavaScriptCore/runtime/CachedTypes.cpp:947
>>> +        }
>> 
>> This can't be right ;-)
>> 
>> I vote for adding some tests at this point to ensure you get the behavior you want.
> 
> damn, I can't believe I did this. I don't know why the compiler didn't complain about it either.
> 
> I agree that sounds reasonable, it was a lot of iterations. Should I make it an API test?

Sounds reasonable. Though the more I think about it I’m not sure quite how to test that we free this object...

Also, it might be worth adding a debug assert that says “!isNewEntry if !isNewAllocation”

[Tadeu Zagallo]

Comment on attachment 362498 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362498&action=review

>>>> Source/JavaScriptCore/runtime/CachedTypes.cpp:947
>>>> +        }
>>> 
>>> This can't be right ;-)
>>> 
>>> I vote for adding some tests at this point to ensure you get the behavior you want.
>> 
>> damn, I can't believe I did this. I don't know why the compiler didn't complain about it either.
>> 
>> I agree that sounds reasonable, it was a lot of iterations. Should I make it an API test?
> 
> Sounds reasonable. Though the more I think about it I’m not sure quite how to test that we free this object...
> 
> Also, it might be worth adding a debug assert that says “!isNewEntry if !isNewAllocation”

Yeah, I'm yet to find a way to test too... I could probably find a way of just encoding/decoding just the handle (without the containing CodeBlock), but I'd have to expose an API for that.

The assertion sounds good, I'll add it.

[Tadeu Zagallo]

Created attachment 362700 [details]
Patch

[Saam Barati]

Comment on attachment 362700 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362700&action=review

r=me

> Source/JavaScriptCore/ChangeLog:12
> +        that we cache the handle instead of the environment.

You should add a sentence why

> Source/JavaScriptCore/runtime/CachedTypes.cpp:380
> +        if (WTF::Optional<ptrdiff_t> offset = encoder.cachedOffsetForPtr(src)) {

Not this patch, but I don’t think the WTF is needed for Optional

> Source/JavaScriptCore/runtime/CachedTypes.cpp:940
> +        CompactVariableEnvironment* environment = m_environment.decode(decoder, isNewAllocation);

This can be a future improvement, but it would be cool if we could skip the hash table lookup for all !isNewAllocation. This could work if we associated a Handle instead of an environment for this pointer. This is sound because a pointer to the same CompactVariableEnvironment* will hash the same. And this is a speed up because hashing CompactVariableEnvironment* isn’t cheap

[Tadeu Zagallo]

Created attachment 362702 [details]
Patch for landing

[Tadeu Zagallo]

(In reply to Saam Barati from comment #19)
> Comment on attachment 362700 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362700&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/ChangeLog:12
> > +        that we cache the handle instead of the environment.
> 
> You should add a sentence why
> 
> > Source/JavaScriptCore/runtime/CachedTypes.cpp:380
> > +        if (WTF::Optional<ptrdiff_t> offset = encoder.cachedOffsetForPtr(src)) {
> 
> Not this patch, but I don’t think the WTF is needed for Optional
> 
> > Source/JavaScriptCore/runtime/CachedTypes.cpp:940
> > +        CompactVariableEnvironment* environment = m_environment.decode(decoder, isNewAllocation);
> 
> This can be a future improvement, but it would be cool if we could skip the
> hash table lookup for all !isNewAllocation. This could work if we associated
> a Handle instead of an environment for this pointer. This is sound because a
> pointer to the same CompactVariableEnvironment* will hash the same. And this
> is a speed up because hashing CompactVariableEnvironment* isn’t cheap

I'll do both of these things in follow up patches today. I'll just land this (with the ChangeLog update you requested) since I need these changes to rebase a couple other patches.

[WebKit Commit Bot]

Comment on attachment 362702 [details]
Patch for landing

Clearing flags on attachment: 362702

Committed r241938: <https://trac.webkit.org/changeset/241938>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48305468>