Bug 186820 – Reduce size of WebCore::URL

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Reduce size of WebCore::URL

[Alex Christensen]

Created attachment 343099 [details]
Patch

[Alex Christensen]

Created attachment 343100 [details]
Patch

[Fujii Hironori]

Comment on attachment 343100 [details]
Patch

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

I'm not a reviewer. This is an informal review.

> Source/WebCore/platform/URL.cpp:167
> +    if (!m_portLength)

Nit: You changed the logic. "m_portLength <= 1" is the equivalent with the orignal because m_portLength includes ':'.

> Source/WebCore/platform/URLParser.cpp:2610
> +        RELEASE_ASSERT(portLength <= URL::maxPortLength);

You added RELEASE_ASSERT here. What will happen with a URL like 'http://example.com:0000080/'?

[Yusuke Suzuki]

Comment on attachment 343100 [details]
Patch

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

> Source/WebCore/platform/URL.h:225
>      bool m_isValid : 1;
>      bool m_protocolIsInHTTPFamily : 1;
>      bool m_cannotBeABaseURL : 1;

I think MSVC does not pack small-sized members if they have different types. (maybe). I think this is why EWS is red.
If it is correct, you can avoid this behavior by changing them to `unsigned`.
At that time, we should take care of the types of these fields. For example, URL::encode and URL::decode should take extra care.

> Source/WebCore/platform/URL.h:245
>  void URL::encode(Encoder& encoder) const

URL::encode does not encode m_cannotBeABaseURL. Is it correct?

> Source/WebCore/platform/URL.h:286
>      if (!decoder.decode(protocolIsInHTTPFamily))

URL::decode does not handle m_cannotBeABaseURL, is it correct?

> Source/WebCore/platform/URLParser.cpp:2701
> -            m_url.m_portEnd = currentPosition(iterator);
> +            unsigned portLength = currentPosition(iterator) - m_url.m_hostEnd;
> +            RELEASE_ASSERT(portLength <= URL::maxPortLength);
> +            m_url.m_portLength = portLength;

I think `m_url.m_portLength = 0` since `iterator.atEnd()` immediately after setting `m_url.m_hostEnd = currentPosition(iterator);`, correct?

> Source/WebCore/platform/URLParser.cpp:2724
> -                m_url.m_portEnd = currentPosition(iterator);
> +                unsigned portLength = currentPosition(iterator) - m_url.m_hostEnd;
> +                RELEASE_ASSERT(portLength <= URL::maxPortLength);
> +                m_url.m_portLength = portLength;

Ditto.

> Source/WebCore/platform/URLParser.cpp:2789
> -            m_url.m_portEnd = currentPosition(iterator);
> +            unsigned portLength = currentPosition(iterator) - m_url.m_hostEnd;
> +            RELEASE_ASSERT(portLength <= URL::maxPortLength);
> +            m_url.m_portLength = portLength;

Ditto.

> Source/WebCore/platform/URLParser.cpp:2803
> -    m_url.m_portEnd = currentPosition(iterator);
> +    unsigned portLength = currentPosition(iterator) - m_url.m_hostEnd;
> +    RELEASE_ASSERT(portLength <= URL::maxPortLength);
> +    m_url.m_portLength = portLength;

Ditto.

[Alex Christensen]

Comment on attachment 343100 [details]
Patch

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

>> Source/WebCore/platform/URL.cpp:167
>> +    if (!m_portLength)
> 
> Nit: You changed the logic. "m_portLength <= 1" is the equivalent with the orignal because m_portLength includes ':'.

This is true, but a URL with a : but no numeric port after is invalid now.

>> Source/WebCore/platform/URLParser.cpp:2610
>> +        RELEASE_ASSERT(portLength <= URL::maxPortLength);
> 
> You added RELEASE_ASSERT here. What will happen with a URL like 'http://example.com:0000080/'?

The port should be normalized to 80.  I'll add a test just in case.

[Alex Christensen]

Created attachment 344775 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 344775 [details]
Patch

Clearing flags on attachment: 344775

Committed r233742: <https://trac.webkit.org/changeset/233742>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42087508>

[Darin Adler]

Comment on attachment 344775 [details]
Patch

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

Seems to me the even more important optimization is to make sure we don’t keep URLs around all the time; how often do we need a pre-parsed string with offsets stored just so we don’t have to scan for special characters again. I kind of wish there was something between String and URL.

> Source/WebCore/platform/URL.h:227
> +    // This is out of order to allign the bits better. The port is after the host.

Typo in the word "align" here.

[Daniel Bates]

Comment on attachment 344775 [details]
Patch

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

> Source/WebCore/platform/URL.h:254
> -    encoder << m_schemeEnd;
> +    encoder << static_cast<bool>(m_cannotBeABaseURL);
> +    encoder << static_cast<unsigned>(m_schemeEnd);
>      encoder << m_userStart;

This is not a binary compatible change (*) and as a result I am hitting the ASSERT_NOT_REACHED() in URL::decode() when I access http://forums.swift.org. The Service Worker registration code that runs in the Storage Process executes and tries to import registration records from disk.

(*) We seem to have inadvertently tied the representation of a URL to what we write when persisting a Service Worker imported script to disk among other things. This logic was added in bug #182444.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 187577

[Daniel Bates]

(In reply to WebKit Commit Bot from comment #12)
> Re-opened since this is blocked by bug 187577

See backtrace and frame variables in bug #187577.

[Chris Dumez]

(In reply to Daniel Bates from comment #13)
> (In reply to WebKit Commit Bot from comment #12)
> > Re-opened since this is blocked by bug 187577
> 
> See backtrace and frame variables in bug #187577.

Looks like we'll see to fix service worker persistence first :/

[Alex Christensen]

(In reply to Chris Dumez from comment #14)
> Looks like we'll see to fix service worker persistence first :/
Couldn't we just increment the version number of the binary format somewhere like we do with the disk cache?

[Chris Dumez]

(In reply to Alex Christensen from comment #15)
> (In reply to Chris Dumez from comment #14)
> > Looks like we'll see to fix service worker persistence first :/
> Couldn't we just increment the version number of the binary format somewhere
> like we do with the disk cache?

I believe the disk cache uses persistent coders that are distinct from IPC coders specifically to avoid this. I think we need a persistent coder for URL and use that one, instead of using the IPC one.

[youenn fablet]

(In reply to Alex Christensen from comment #15)
> (In reply to Chris Dumez from comment #14)
> > Looks like we'll see to fix service worker persistence first :/
> Couldn't we just increment the version number of the binary format somewhere
> like we do with the disk cache?

We should do that and serialize URLs as strings in the persistent script map at the same time.

[Alex Christensen]

Created attachment 344877 [details]
Patch

[Alex Christensen]

Created attachment 344878 [details]
Patch

[youenn fablet]

Comment on attachment 344878 [details]
Patch

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

> Source/WebCore/platform/URL.cpp:461
> +    URLParser parser(m_string.left(m_hostEnd) + m_string.substring(m_hostEnd + m_portLength));

That is not great to create three strings here.
Could we try to use StringViews instead of left and substring?

> Source/WebCore/platform/URL.cpp:473
> +    URLParser parser(makeString(m_string.left(portStart), (colonNeeded ? ":" : ""), String::number(i), m_string.substring(m_hostEnd + m_portLength)));

Ditto here.

> Source/WebCore/platform/URL.cpp:513
> +    builder.append(m_string.substring(m_hostEnd + m_portLength));

I believe StringBuilder is taking StringView as input.

[Alex Christensen]

Created attachment 344883 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r233789

[Truitt Savell]

An API test failure was introduced in r233789 <https://trac.webkit.org/changeset/233789/webkit>

Test Output:

Test suite failed

Failed

    TestWebKitAPI.WebKit.CustomDataStorePathsVersusCompletionHandlers
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:264
        Value of: 1
        Expected: (int)dataRecords.count
        Which is: 0
        
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:273
        Value of: 1
        Expected: (int)dataRecords.count
        Which is: 0

[Chris Dumez]

(In reply to Truitt Savell from comment #23)
> An API test failure was introduced in r233789
> <https://trac.webkit.org/changeset/233789/webkit>
> 
> Test Output:
> 
> Test suite failed
> 
> Failed
> 
>     TestWebKitAPI.WebKit.CustomDataStorePathsVersusCompletionHandlers
>         
>        
> /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/WebsiteDataStoreCustomPaths.mm:264
>         Value of: 1
>         Expected: (int)dataRecords.count
>         Which is: 0
>         
>         
>        
> /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/WebsiteDataStoreCustomPaths.mm:273
>         Value of: 1
>         Expected: (int)dataRecords.count
>         Which is: 0

We need to update the pre-baked service worker registration database that the API tests are using, since the format has changed in this patch.

[Chris Dumez]

(In reply to Chris Dumez from comment #24)
> (In reply to Truitt Savell from comment #23)
> > An API test failure was introduced in r233789
> > <https://trac.webkit.org/changeset/233789/webkit>
> > 
> > Test Output:
> > 
> > Test suite failed
> > 
> > Failed
> > 
> >     TestWebKitAPI.WebKit.CustomDataStorePathsVersusCompletionHandlers
> >         
> >        
> > /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> > WebKitCocoa/WebsiteDataStoreCustomPaths.mm:264
> >         Value of: 1
> >         Expected: (int)dataRecords.count
> >         Which is: 0
> >         
> >         
> >        
> > /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> > WebKitCocoa/WebsiteDataStoreCustomPaths.mm:273
> >         Value of: 1
> >         Expected: (int)dataRecords.count
> >         Which is: 0
> 
> We need to update the pre-baked service worker registration database that
> the API tests are using, since the format has changed in this patch.

Tools//TestWebKitAPI/Tests/WebKitCocoa/SimpleServiceWorkerRegistrations-2.sqlite3
Tools//TestWebKitAPI/Tests/WebKitCocoa/SimpleServiceWorkerRegistrations-2.sqlite3-shm
Tools//TestWebKitAPI/Tests/WebKitCocoa/SimpleServiceWorkerRegistrations-2.sqlite3-wal

[Chris Dumez]

(In reply to Chris Dumez from comment #25)
> (In reply to Chris Dumez from comment #24)
> > (In reply to Truitt Savell from comment #23)
> > > An API test failure was introduced in r233789
> > > <https://trac.webkit.org/changeset/233789/webkit>
> > > 
> > > Test Output:
> > > 
> > > Test suite failed
> > > 
> > > Failed
> > > 
> > >     TestWebKitAPI.WebKit.CustomDataStorePathsVersusCompletionHandlers
> > >         
> > >        
> > > /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> > > WebKitCocoa/WebsiteDataStoreCustomPaths.mm:264
> > >         Value of: 1
> > >         Expected: (int)dataRecords.count
> > >         Which is: 0
> > >         
> > >         
> > >        
> > > /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> > > WebKitCocoa/WebsiteDataStoreCustomPaths.mm:273
> > >         Value of: 1
> > >         Expected: (int)dataRecords.count
> > >         Which is: 0
> > 
> > We need to update the pre-baked service worker registration database that
> > the API tests are using, since the format has changed in this patch.
> 
> Tools//TestWebKitAPI/Tests/WebKitCocoa/SimpleServiceWorkerRegistrations-2.
> sqlite3
> Tools//TestWebKitAPI/Tests/WebKitCocoa/SimpleServiceWorkerRegistrations-2.
> sqlite3-shm
> Tools//TestWebKitAPI/Tests/WebKitCocoa/SimpleServiceWorkerRegistrations-2.
> sqlite3-wal

I am looking into this now.

[Chris Dumez]

Committed r233798: <https://trac.webkit.org/changeset/233798>

[Antti Koivisto]

Comment on attachment 344883 [details]
Patch

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

> Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.h:111
>      // Incrementing this number will delete all existing cache content for everyone. Do you really need to do it?
> -    static const unsigned version = 12;
> +    static const unsigned version = 13;

Did you, really?

[Fujii Hironori]

(In reply to Antti Koivisto from comment #28)
> Did you, really?

Did you see Bug 187577 mentioned in above comments?