Closed
Bug 1444151
Opened 6 years ago
Closed 6 years ago
Simplify MozURL and avoid unnecessary allocations
Categories
(Core :: Networking, enhancement, P5)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: nika, Assigned: nika)
Details
(Whiteboard: [necko-triaged])
Attachments
(4 files)
5.70 KB,
patch
|
valentin
:
review+
|
Details | Diff | Splinter Review |
920 bytes,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
61.70 KB,
patch
|
valentin
:
review+
|
Details | Diff | Splinter Review |
15.72 KB,
patch
|
valentin
:
review+
|
Details | Diff | Splinter Review |
Currently mozurl performs 2 allocations: 1 for the C++ wrapper, and 1 for the Rust url object. This patch does the legwork to make it only do one, and simplifies a bunch of code in the process. I also clean up the API of MozURL a bit.
Assignee | ||
Comment 1•6 years ago
|
||
MozReview-Commit-ID: LA77KrTrRbk
Attachment #8957242 -
Flags: review?(valentin.gosu)
Assignee | ||
Comment 2•6 years ago
|
||
MozReview-Commit-ID: EpgU4nKkSJs
Attachment #8957243 -
Flags: review?(nfroyd)
Assignee | ||
Comment 3•6 years ago
|
||
This patch rewrites the rust-url-capi crate as the mozurl crate, which provides a threadsafe MozURL object which is compatible with the previous MozURL class. Creating a MozURL this way performs a single allocation, which contains only a rust-url Url object and an atomic refcnt, however it is fully compatible with the C++ RefPtr type. This patch also exposes methods for accessing dependent substrings of the serialized spec, meaning that string copies can be avoided in many situations when inspecting attributes of the MozURL. MozReview-Commit-ID: GSoDWlQG4XP
Attachment #8957244 -
Flags: review?(valentin.gosu)
Assignee | ||
Comment 4•6 years ago
|
||
MozReview-Commit-ID: 1q1XuRRHr4x
Attachment #8957245 -
Flags: review?(valentin.gosu)
Updated•6 years ago
|
Attachment #8957243 -
Flags: review?(nfroyd) → review+
Updated•6 years ago
|
Attachment #8957242 -
Flags: review?(valentin.gosu) → review+
Updated•6 years ago
|
Attachment #8957245 -
Flags: review?(valentin.gosu) → review+
Comment 5•6 years ago
|
||
Comment on attachment 8957244 [details] [diff] [review] Part 3: Only create a single allocation for MozURL objects, which is managed by rust Review of attachment 8957244 [details] [diff] [review]: ----------------------------------------------------------------- Great job. Just a couple minor nits. ::: Cargo.lock @@ +1124,5 @@ > +version = "0.0.1" > +dependencies = [ > + "nserror 0.1.0", > + "nsstring 0.1.0", > + "url 1.6.0 (registry+https://github.com/rust-lang/crates.io-index)", Could you also update to url 1.7.0 ? ::: netwerk/base/mozurl/MozURL.h @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public nit: this file is copied instead of moved. Not sure if that was your intent.
Attachment #8957244 -
Flags: review?(valentin.gosu) → review+
Updated•6 years ago
|
Priority: -- → P5
Whiteboard: [necko-triaged]
Pushed by nika@thelayzells.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/beff5418f8e5 Part 1: Remove unused rust-url-capi tests, r=valentin https://hg.mozilla.org/integration/mozilla-inbound/rev/06e75a3b8d8e Part 2: Make Rust's RefPtr::forget safe, r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/a0c94bfa5a40 Part 3: Only create a single allocation for MozURL objects, which is managed by rust, r=valentin https://hg.mozilla.org/integration/mozilla-inbound/rev/239b9760cb67 Part 4: Remove the now-unnecessary xpcom-style segment getters, r=valentin https://hg.mozilla.org/integration/mozilla-inbound/rev/c217f857942f Part 5: Update rust-url to 1.7.0, r=valentin
Comment 7•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/beff5418f8e5 https://hg.mozilla.org/mozilla-central/rev/06e75a3b8d8e https://hg.mozilla.org/mozilla-central/rev/a0c94bfa5a40 https://hg.mozilla.org/mozilla-central/rev/239b9760cb67 https://hg.mozilla.org/mozilla-central/rev/c217f857942f
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•