[JSC] Optimize layout of SourceProvider to reduce padding
Created attachment 344528 [details] Patch
Attachment 344528 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/parser/SourceProvider.h:38: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 344528 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344528&action=review r=me with suggested changes. > Source/JavaScriptCore/parser/SourceProvider.h:90 > + uintptr_t m_id { 0 }; I think m_id does not need to be a uintptr_t. You can make it a uint32_t and move it above to save another 8 bytes (down to 64). If we have more than 4G of unique SourceProviders, I think we would have other memory problems long before we get to exhausting the ids. Also: 1. in SourceProvider::getID(), change nextProviderID into a static uint32_t. 2. in SourceProvider::getID(), add a RELEASE_ASSERT(m_id) to make sure that nextProviderID does not overflow, and we do not fail silently. This should never happen, but it's good to be explicit, and the RELEASE_ASSERT will also document this constraint.
Comment on attachment 344528 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344528&action=review >> Source/JavaScriptCore/parser/SourceProvider.h:90 >> + uintptr_t m_id { 0 }; > > I think m_id does not need to be a uintptr_t. You can make it a uint32_t and move it above to save another 8 bytes (down to 64). If we have more than 4G of unique SourceProviders, I think we would have other memory problems long before we get to exhausting the ids. > > Also: > 1. in SourceProvider::getID(), change nextProviderID into a static uint32_t. > 2. in SourceProvider::getID(), add a RELEASE_ASSERT(m_id) to make sure that nextProviderID does not overflow, and we do not fail silently. This should never happen, but it's good to be explicit, and the RELEASE_ASSERT will also document this constraint. I think the current code is OK because, 1. Even if we convert m_id to uint32_t, we cannot get additional 8 bytes. Since this SourceProvider class is RefCounted, we have 4 bytes m_refCount in the header. We make m_refCount + m_sourceType + m_validate 8 bytes now. 2. If we repeatedly execute `eval(dynamicString)`, we may get many SourceProviders. Since we cannot get additional space b/c of (1), I think using the current code is fine. What do you think of?
(In reply to Yusuke Suzuki from comment #4) > Comment on attachment 344528 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=344528&action=review > > >> Source/JavaScriptCore/parser/SourceProvider.h:90 > >> + uintptr_t m_id { 0 }; > > > > I think m_id does not need to be a uintptr_t. You can make it a uint32_t and move it above to save another 8 bytes (down to 64). If we have more than 4G of unique SourceProviders, I think we would have other memory problems long before we get to exhausting the ids. > > > > Also: > > 1. in SourceProvider::getID(), change nextProviderID into a static uint32_t. > > 2. in SourceProvider::getID(), add a RELEASE_ASSERT(m_id) to make sure that nextProviderID does not overflow, and we do not fail silently. This should never happen, but it's good to be explicit, and the RELEASE_ASSERT will also document this constraint. > > I think the current code is OK because, > > 1. Even if we convert m_id to uint32_t, we cannot get additional 8 bytes. > Since this SourceProvider class is RefCounted, we have 4 bytes m_refCount in > the header. We make m_refCount + m_sourceType + m_validate 8 bytes now. > 2. If we repeatedly execute `eval(dynamicString)`, we may get many > SourceProviders. Since we cannot get additional space b/c of (1), I think > using the current code is fine. > > What do you think of? I missed the RefCounted base class. I know that it's not caused by your patch but can you still add a RELEASE_ASSERT(m_id) in SourceProvider::getID()? I still think it's a good idea to not fail silently. I'm ok with this patch otherwise. Thanks.
Comment on attachment 344528 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344528&action=review >>>> Source/JavaScriptCore/parser/SourceProvider.h:90 >>>> + uintptr_t m_id { 0 }; >>> >>> I think m_id does not need to be a uintptr_t. You can make it a uint32_t and move it above to save another 8 bytes (down to 64). If we have more than 4G of unique SourceProviders, I think we would have other memory problems long before we get to exhausting the ids. >>> >>> Also: >>> 1. in SourceProvider::getID(), change nextProviderID into a static uint32_t. >>> 2. in SourceProvider::getID(), add a RELEASE_ASSERT(m_id) to make sure that nextProviderID does not overflow, and we do not fail silently. This should never happen, but it's good to be explicit, and the RELEASE_ASSERT will also document this constraint. >> >> I think the current code is OK because, >> >> 1. Even if we convert m_id to uint32_t, we cannot get additional 8 bytes. Since this SourceProvider class is RefCounted, we have 4 bytes m_refCount in the header. We make m_refCount + m_sourceType + m_validate 8 bytes now. >> 2. If we repeatedly execute `eval(dynamicString)`, we may get many SourceProviders. Since we cannot get additional space b/c of (1), I think using the current code is fine. >> >> What do you think of? > > I missed the RefCounted base class. I know that it's not caused by your patch but can you still add a RELEASE_ASSERT(m_id) in SourceProvider::getID()? I still think it's a good idea to not fail silently. I'm ok with this patch otherwise. Thanks. Sounds reasonable. I'll add it!
Comment on attachment 344528 [details] Patch Attachment 344528 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8468854 New failing tests: animations/needs-layout.html
Created attachment 344530 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Committed r233626: <https://trac.webkit.org/changeset/233626>
<rdar://problem/41945932>