Closed Bug 1444151 Opened 6 years ago Closed 6 years ago

Simplify MozURL and avoid unnecessary allocations

Categories

(Core :: Networking, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: nika, Assigned: nika)

Details

(Whiteboard: [necko-triaged])

Attachments

(4 files)

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.
MozReview-Commit-ID: LA77KrTrRbk
Attachment #8957242 - Flags: review?(valentin.gosu)
MozReview-Commit-ID: EpgU4nKkSJs
Attachment #8957243 - Flags: review?(nfroyd)
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)
MozReview-Commit-ID: 1q1XuRRHr4x
Attachment #8957245 - Flags: review?(valentin.gosu)
Attachment #8957243 - Flags: review?(nfroyd) → review+
Attachment #8957242 - Flags: review?(valentin.gosu) → review+
Attachment #8957245 - Flags: review?(valentin.gosu) → review+
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+
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
You need to log in before you can comment on or make changes to this bug.