Closed Bug 1359353 Opened 7 years ago Closed 7 years ago

Mutable references to XPCOM strings in Rust should deref to mutable slices

Categories

(Core :: XPCOM, enhancement)

Product:
Core
Component:
XPCOM
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: hsivonen, Assigned: hsivonen)

References

Details

Attachments

(1 file)

Bug 1359353 - Make the backing buffers of XPCOM strings available as mutable slices. .
59 bytes, text/x-review-board-request
nika
: review+
Details 
Patch coming up.
Use case: Letting encoding_rs write directly into the buffer of XPCOM strings.

The patch doesn't have tests, because it's not clear to me how to properly update the cargo lock file that ./mach gtest complains about. Mystor, any advice on what ./mach incantation updates the lock file?
Flags: needinfo?(michael)
Summary: Mutable references XPCOM strings in Rust should deref to mutable slices → Mutable references to XPCOM strings in Rust should deref to mutable slices
Comment on attachment 8861345 [details]
Bug 1359353 - Make the backing buffers of XPCOM strings available as mutable slices. .

https://reviewboard.mozilla.org/r/133338/#review136278

> The patch doesn't have tests, because it's not clear to me how to properly update the cargo lock file that ./mach gtest complains about. Mystor, any advice on what ./mach incantation updates the lock file?

I'm not sure what the best way to update that lockfile is either. As far as I know there is no way to update it with a simple `./mach` incantation, froydnj would know more.

Personally, I handle the updates by commenting out the line `cargo_build_flags += --frozen` in `config/rules.mk` and then running both `./mach build` and `./mach gtest`. I then uncomment the line again before committing - gross I know. I don't know why you would have to update any lockfiles however. There is already a test C++ and rust file in `xpcom/rust/nsstring/test` which you should be able to add the tests for this feature to the end of. Were you planning to test encoding_rs directly in the nsstring tests?

::: xpcom/rust/nsstring/src/lib.rs:309
(Diff revision 1)
>                      }
>                  }
>              }
>          }
>  
> +        impl DerefMut for $AString {

$begin_writing can perform an O(n) COW copy into a new memory buffer to enable writing into the backing memory for the given nsA[C]String. I don't think that this potentially expensive operation should be hidden behind an implicit dereference operation. 

Please move this logic into a method on `$AString` which has the same behavior, perhaps `to_mut()` to use the same method name as `Cow<'a, str>`? I would also be OK with using `begin_writing` for symmetry with C++. 

Please also add documentation to the new method, perhaps:

/// Get a `&mut` reference to the backing data for this `nsA[C]String`. This method will allocate and copy if the current backing buffer is immutable or shared. 

For consistency with the other methods on $AString, please also expose the fallible version of BeginWriting().

::: xpcom/rust/nsstring/src/lib.rs:323
(Diff revision 1)
> +                        debug_assert!(this.length == 0);
> +                        // Use an arbitrary non-null value as the pointer
> +                        slice::from_raw_parts_mut(0x1 as *mut $char_t, 0)
> +                    } else {
> +                        let len = this.length as usize;
> +                        let self_again: &mut $AString = mem::transmute(this);

This dance seems unnecessary to me. Why not do:

```
unsafe {
    let len = self.len();
    if len == 0 {
        // Use an arbitrary non-null value as the pointer
        slice::from_raw_parts_mut(0x1 as *mut $char_t, 0)
    } else {
        slice::from_raw_parts_mut($begin_writing(self), len)
    }
}
```
Attachment #8861345 - Flags: review-
(In reply to Henri Sivonen (:hsivonen) from comment #2)
> The patch doesn't have tests, because it's not clear to me how to properly
> update the cargo lock file that ./mach gtest complains about. Mystor, any
> advice on what ./mach incantation updates the lock file?

Nathan, do you know if there's a mach command for updating lockfiles? I have just been editing rules.mk and rebuilding.
Flags: needinfo?(michael) → needinfo?(nfroyd)
(In reply to Michael Layzell [:mystor] from comment #4)
> (In reply to Henri Sivonen (:hsivonen) from comment #2)
> > The patch doesn't have tests, because it's not clear to me how to properly
> > update the cargo lock file that ./mach gtest complains about. Mystor, any
> > advice on what ./mach incantation updates the lock file?
> 
> Nathan, do you know if there's a mach command for updating lockfiles? I have
> just been editing rules.mk and rebuilding.

We don't have a mach command; you should be able to do some variant of `cargo update --manifest-path ... -p gkrust`, and probably the same for gkrust-gtest.  That reminds me, `mach vendor rust` is probably broken since we moved the lock files around...
Flags: needinfo?(nfroyd)
(In reply to Michael Layzell [:mystor] from comment #3)
> $begin_writing can perform an O(n) COW copy into a new memory buffer to
> enable writing into the backing memory for the given nsA[C]String. I don't
> think that this potentially expensive operation should be hidden behind an
> implicit dereference operation. 

I'd agree if this was a Rust-native type whose costs were generally overt. However, the general behavior of XPCOM strings is that the copy happens implicitly in various situations. In that context, it seems weird to have to be more explicit on the Rust side.

Not the best precedent to make my case, since the precedent was written by me, but on the C++ side, XPCOM strings convert implicitly to writable Spans:
https://searchfox.org/mozilla-central/source/xpcom/string/nsTSubstring.h#931
(In reply to Henri Sivonen (:hsivonen) from comment #6)
> (In reply to Michael Layzell [:mystor] from comment #3)
> > $begin_writing can perform an O(n) COW copy into a new memory buffer to
> > enable writing into the backing memory for the given nsA[C]String. I don't
> > think that this potentially expensive operation should be hidden behind an
> > implicit dereference operation. 
> 
> I'd agree if this was a Rust-native type whose costs were generally overt.
> However, the general behavior of XPCOM strings is that the copy happens
> implicitly in various situations. In that context, it seems weird to have to
> be more explicit on the Rust side.
> 
> Not the best precedent to make my case, since the precedent was written by
> me, but on the C++ side, XPCOM strings convert implicitly to writable Spans:
> https://searchfox.org/mozilla-central/source/xpcom/string/nsTSubstring.h#931

I understand that on the C++ side expensive operations can happen implicitly, and it might seem strange to have there be differences, but no matter what there will be API differences. For example, the Rust code does not have implicit copy constructors or any mechanism for implicit conversion constructors like the one you have implemented for Span.

I would be greatly inclined to keep the API explicit on the rust side. It doesn't reduce the clarity of the consumer of the code, makes it easy to switch to a fallible implementation if we need to, and doesn't violate rust consumers' expectations of what a DerefMut implementation should do.

IMO when writing rust code people have different expectations for what the code which they are interacting with is hiding from them and this is something which would be unexpected.
Comment on attachment 8861345 [details]
Bug 1359353 - Make the backing buffers of XPCOM strings available as mutable slices. .

https://reviewboard.mozilla.org/r/133338/#review136278

> $begin_writing can perform an O(n) COW copy into a new memory buffer to enable writing into the backing memory for the given nsA[C]String. I don't think that this potentially expensive operation should be hidden behind an implicit dereference operation. 
> 
> Please move this logic into a method on `$AString` which has the same behavior, perhaps `to_mut()` to use the same method name as `Cow<'a, str>`? I would also be OK with using `begin_writing` for symmetry with C++. 
> 
> Please also add documentation to the new method, perhaps:
> 
> /// Get a `&mut` reference to the backing data for this `nsA[C]String`. This method will allocate and copy if the current backing buffer is immutable or shared. 
> 
> For consistency with the other methods on $AString, please also expose the fallible version of BeginWriting().

Used `to_mut()` and added explicit variant, too.

> This dance seems unnecessary to me. Why not do:
> 
> ```
> unsafe {
>     let len = self.len();
>     if len == 0 {
>         // Use an arbitrary non-null value as the pointer
>         slice::from_raw_parts_mut(0x1 as *mut $char_t, 0)
>     } else {
>         slice::from_raw_parts_mut($begin_writing(self), len)
>     }
> }
> ```

Used this form.
Comment on attachment 8861345 [details]
Bug 1359353 - Make the backing buffers of XPCOM strings available as mutable slices. .

https://reviewboard.mozilla.org/r/133338/#review136278

> Used `to_mut()` and added explicit variant, too.

s/explicit/fallible/
Comment on attachment 8861345 [details]
Bug 1359353 - Make the backing buffers of XPCOM strings available as mutable slices. .

https://reviewboard.mozilla.org/r/133338/#review136998

Awesome, thanks!
Attachment #8861345 - Flags: review?(michael) → review+
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe5a5bcd4cb3
Make the backing buffers of XPCOM strings available as mutable slices. r=mystor.
https://hg.mozilla.org/mozilla-central/rev/fe5a5bcd4cb3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: