Similar to https://bugs.webkit.org/show_bug.cgi?id=194583, but for the bytecode cache.
Created attachment 362118 [details] Patch
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.
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.
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.
Created attachment 362435 [details] Patch
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.
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.
Created attachment 362485 [details] Patch
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.
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
Created attachment 362498 [details] Patch
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!
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.
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.
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?
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”
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.
Created attachment 362700 [details] Patch
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
Created attachment 362702 [details] Patch for landing
(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.
Comment on attachment 362702 [details] Patch for landing Clearing flags on attachment: 362702 Committed r241938: <https://trac.webkit.org/changeset/241938>
All reviewed patches have been landed. Closing bug.
<rdar://problem/48305468>