Bug 118955 - Strive to keep using the original request KURL instead of replacing it with identical copies.
Summary: Strive to keep using the original request KURL instead of replacing it with i...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: InRadar, Performance
Depends on:
Blocks:
 
Reported: 2013-07-21 12:17 PDT by Andreas Kling
Modified: 2013-08-02 06:57 PDT (History)
8 users (show)

See Also:

Attachments
Patch idea (3.60 KB, patch)
2013-07-21 12:18 PDT, Andreas Kling 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2013-07-21 12:17:13 PDT 
Strive to keep using the original request KURL instead of replacing it with identical copies.
Comment 1 Radar WebKit Bug Importer 2013-07-21 12:17:30 PDT 
<rdar://problem/14504835>
Comment 2 Andreas Kling 2013-07-21 12:18:59 PDT 
Created attachment 207211 [details]
Patch idea
Comment 3 Andreas Kling 2013-07-21 16:06:04 PDT 
This feels kinda hackish. Would love to hear from someone with loader expertise.
Comment 4 Sam Weinig 2013-07-21 18:26:53 PDT 
I agree, it feel hackish.
Comment 5 Andreas Kling 2013-07-22 07:59:21 PDT 
This is a 963 kB saver on theverge.com as well. Basically any site that uses huge data: URIs will see great improvement from fixing this.
Comment 6 Brady Eidson 2013-07-22 09:21:20 PDT 
Comment on attachment 207211 [details]
Patch idea

Each of these 3 call sites focus on setting a KURL to another KURL.

To be less hacky and to make experimenting with this optimization more centralized, I wonder if KURL's operator== should do this itself?
Comment 7 Brady Eidson 2013-07-22 09:22:29 PDT 
That would definitely give us maximum memory savings, but would possibly be a performance hit... at which point we could scale it back and only do the optimization conditionally.
Comment 8 Andreas Kling 2013-07-22 09:24:01 PDT 
(In reply to comment #6)
> (From update of attachment 207211 [details])
> Each of these 3 call sites focus on setting a KURL to another KURL.
> 
> To be less hacky and to make experimenting with this optimization more centralized, I wonder if KURL's operator== should do this itself?

Something like.. if KURL::operator== will return true, it should first set m_string of both KURLs to whichever string has more refs?
Comment 9 Brady Eidson 2013-07-22 09:28:38 PDT 
(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 207211 [details] [details])
> > Each of these 3 call sites focus on setting a KURL to another KURL.
> > 
> > To be less hacky and to make experimenting with this optimization more centralized, I wonder if KURL's operator== should do this itself?
> 
> Something like.. if KURL::operator== will return true, it should first set m_string of both KURLs to whichever string has more refs?

Sure.

Looking at operator== right now, it's so simplistic...  those days are over!  :)
Comment 10 Brady Eidson 2013-07-22 09:29:25 PDT 
LOL - I bet doing this will uncover some threading errors.

Perfect time for this!
Comment 11 Brady Eidson 2013-07-22 09:30:11 PDT 
CRAP dude.  sorry.  I'm not awake yet.

I meant operator=, not operator==.
Comment 12 Andreas Kling 2013-07-22 09:31:53 PDT 
(In reply to comment #11)
> CRAP dude.  sorry.  I'm not awake yet.
> 
> I meant operator=, not operator==.

That makes a bit more sense yeah :)
Comment 13 Andreas Kling 2013-08-02 06:57:51 PDT 
Comment on attachment 207211 [details]
Patch idea

Retracting this patch for now.