Reduce size of WebCore::URL
Created attachment 343099 [details] Patch
Created attachment 343100 [details] Patch
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/'?
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.
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.
Created attachment 344775 [details] Patch
Comment on attachment 344775 [details] Patch Clearing flags on attachment: 344775 Committed r233742: <https://trac.webkit.org/changeset/233742>
All reviewed patches have been landed. Closing bug.
<rdar://problem/42087508>
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.
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.
Re-opened since this is blocked by bug 187577
(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.
(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 :/
(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?
(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.
(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.
Created attachment 344877 [details] Patch
Created attachment 344878 [details] Patch
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.
Created attachment 344883 [details] Patch
http://trac.webkit.org/r233789
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
(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.
(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
(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.
Committed r233798: <https://trac.webkit.org/changeset/233798>
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?
(In reply to Antti Koivisto from comment #28) > Did you, really? Did you see Bug 187577 mentioned in above comments?