Open
Bug 1449559
Opened 7 years ago
Updated 2 years ago
Add nsIURI::GetSpecHash method to calculate hash from spec
Categories
(Core :: Networking, enhancement, P3)
Core
Networking
Tracking
()
NEW
People
(Reporter: aosmond, Assigned: aosmond)
References
Details
(Whiteboard: [necko-triaged])
Attachments
(1 file, 1 obsolete file)
There are a few places we don't particularly care what the URI spec is, but we do want a hash representation of it. Ideally we could avoid this temporary copy, especially for data URIs which can be rather large.
https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/netwerk/base/nsURIHashKey.h#52
https://searchfox.org/mozilla-central/rev/028cd8342899357d80fafba2d528a0fd5819e316/image/ImageURL.h#132
ImageURL currently stores the spec for off-main-thread use, but this will be removed in bug 920630. After that lands, we will only need a temporary copy, just like nsURIHashKey::HashKey.
Assignee | ||
Comment 1•7 years ago
|
||
Updated•7 years ago
|
Priority: -- → P1
Whiteboard: [necko-triaged]
Assignee | ||
Comment 2•7 years ago
|
||
I will break out the ImageCacheKey bits if this is ready to land before bug 920630.
Attachment #8963103 -
Attachment is obsolete: true
Attachment #8963555 -
Flags: review?(valentin.gosu)
Comment 3•7 years ago
|
||
Comment on attachment 8963555 [details] [diff] [review]
0006-Bug-1449559-Add-nsIURI-GetSpecHash-to-generate-a-has.patch, v2
Review of attachment 8963555 [details] [diff] [review]:
-----------------------------------------------------------------
(In reply to Andrew Osmond [:aosmond] from comment #0)
> ImageURL currently stores the spec for off-main-thread use, but this will be
> removed in bug 920630. After that lands, we will only need a temporary copy,
> just like nsURIHashKey::HashKey.
URLs are now thread-safe, so we don't need to store the spec anymore. You can call nsIURI::GetSpec() on any thread.
::: image/ImageCacheKey.cpp
@@ +63,5 @@
> aRv = mURI->GetRef(mRef);
> NS_ENSURE_SUCCESS_VOID(aRv);
> mHash = HashGeneric(*mBlobSerial, HashString(mRef));
> } else {
> + aRv = mURI->GetSpecHash(&mHash);
Is this an especially hot code-path? Can you do some micro-benchmarking to see how much this actually improves things? And see if there are multiple places in the code that would benefit from having this method?
Because if this would greatly improve things, we could also consider having the value of the hash in the URI, and update it every time we change the URI. This would make the speed of hashing O(1). But of course, it depends on how often we do it. If it's rarely, then maybe it's not worth the overhead of recomputing the hash every time.
::: netwerk/base/nsIURI.idl
@@ +92,5 @@
> + * users of nsIURI to call GetSpec, thereby saving extra allocating and
> + * freeing. Note that this may not produce the same result as GetSpec and
> + * HashString.
> + */
> + readonly attribute uint32_t specHash;
My main problem with this is that consumers will now call specHash without considering what they actually want. They may want the hash of the spec, or the hash of the "spec without ref".
I would prefer an approach closer to:
uint32_t getSpecHash(in bool includeRef);
Attachment #8963555 -
Flags: review?(valentin.gosu)
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•