Bug 187440

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>