Bug 36794

Created attachment 51985 [details] Patch

Created attachment 51986 [details] Patch

Attachment 51986 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1634026

Created attachment 51991 [details] Patch

Attachment 51991 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1614078

Comment on attachment 51991 [details] Patch Anyway, I'll make it build before landing, obviously.

What is the advantage of adding another string type over simply using KURL in more places? If we are concerned about the footprint of a KURL, then perhaps we could optimize it. I can imagine it being nearly as cheap as a String object. We could leverage the work that was done to allocate StringImpl with the UChar array, to allow us to store the additional indices held by a KURL object inline with the URL characters. This way, we achieve a small footprint for a KURL object, and avoid having to re-parse and re-canonicalize URL strings. We can have a simple rule that encourages use of KURL everywhere we reference absolute URLs.

> If we are concerned about the footprint of a KURL, then perhaps we could > optimize it. Yes, that was the concern. It would be even better, if it can be done.

(In reply to comment #8) > > If we are concerned about the footprint of a KURL, then perhaps we could > > optimize it. > > Yes, that was the concern. It would be even better, if it can be done. It seems fairly trivial to store the indices and IsValid bit alongside the character array in memory such that a null KURL would only require sizeof(void*) memory. We could just invent a variant of StringImpl::createUninitialized that provides for a little extra storage in the allocation.

Comment on attachment 51991 [details] Patch No love for this patch. I can re-spin it incorporating the comments herein if folks think this is the right direction. Otherwise, we can go more in the direction that fishd recommends.

(In reply to comment #10) > Otherwise, we can go more in the > direction that fishd recommends. We should find out how practical his idea is. If there is already an existing String or AtomicString, the converting it to a KURL would presumably require copying it even if we do squirrel the URL bits away inside the string buffer. But generally speaking if we can make a KURL almost the same cost as a string then we could make our code more readable. And also, someone should take the damned "K" out ;-)

> We should find out how practical his idea is. I can try sketching out some code for this. > If there is already an existing String or AtomicString, the converting it to a > KURL would presumably require copying it even if we do squirrel the URL bits > away inside the string buffer. But generally speaking if we can make a KURL > almost the same cost as a string then we could make our code more readable. You can avoid the extra copy with another malloc, but that doesn't seem like a good trade off. Is the memory concern really with empty KURLs? I wish there was a good way to measure how much memory we're talking about here. > And also, someone should take the damned "K" out ;-) :)

(In reply to comment #12) > You can avoid the extra copy with another malloc, but that doesn't seem like a > good trade off. Is the memory concern really with empty KURLs? I wish there > was a good way to measure how much memory we're talking about here. At least we could start by figuring out how much memory a KURL uses and how much memory the String for that KURL uses on some representative platform.

I should add, that I also find Maciej's idea interesting. To summarize: 1- URLString contains a canonical, absolute URL string. 2- ParsedURL is constructed when you need to access/replace components from an URLString. Constructing an URLString from a String would require instantiating a ParsedURL so that canonicalization could be done. Constructing a ParsedURL from an URLString would be cheap because ParsedURL could assume the input is already canonical. In such a world, KURL disappears in favor of either URLString or ParsedURL. This proposal might result in some hot-spots in the code that lead to us passing around ParsedURL objects, but I suspect those will be the exception to the norm. Reducing the memory size of KURL so that it can be used in more places is probably a fairly low cost and incremental change to the code base, but it may be unnecessary work given Maciej's idea above.

Darin, I’m a little lost. Starting implementation on what you call “Maciej’s idea” is what this bug was all about. How will we decide which way to go?

(In reply to comment #15) > Darin, I’m a little lost. Starting implementation on what you call “Maciej’s > idea” is what this bug was all about. How will we decide which way to go? Right... when I originally commented in this bug, it was not clear to me that this was a first step toward that plan. There's no mention of it here. After some discussion on #webkit, it became clear to me that that was the case. Based on some of the comments in this bug (in particular comment #8), it sounds like the idea of optimizing KURL so that it could be used everywhere (instead of having two representations for URLs) had not been fully considered. How to best proceed is a good question. I can see pros and cons both ways.