Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ScriptOrigin::text duplicates the string contents #27254

Closed
jdm opened this issue Jul 13, 2020 · 10 comments
Closed

ScriptOrigin::text duplicates the string contents #27254

jdm opened this issue Jul 13, 2020 · 10 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Jul 13, 2020

Since these scripts can be large minified scripts, if there's a way to borrow the result instead that would be preferable.

@camelid
Copy link
Contributor

@camelid camelid commented Jul 13, 2020

I don't know the context, but what about a Cow?

@jdm
Copy link
Member Author

@jdm jdm commented Jul 13, 2020

I don't think we ever need to mutate the contents of the string, so a straight borrow is probably all that would be required.

@camelid
Copy link
Contributor

@camelid camelid commented Jul 13, 2020

So this would just entail updating all the usages so that it works with a reference instead of an owned value then?

@jdm
Copy link
Member Author

@jdm jdm commented Jul 14, 2020

Yes, this would require investigating whether it's possible to avoid using an owned value in the code that relies on this method.

@camelid
Copy link
Contributor

@camelid camelid commented Jul 14, 2020

Is it okay to introduce a lifetime to ModuleTree so that the text field can be DomRefCell<&'a DOMString> instead of DomRefCell<DOMString>? Or is there a better way to do it that won't affect other code?

@jdm
Copy link
Member Author

@jdm jdm commented Jul 14, 2020

One other option would be storing Rc<DOMString>. I suspect introducing a lifetime might be tricky.

@camelid
Copy link
Contributor

@camelid camelid commented Jul 14, 2020

Yeah, that's kind of what I was thinking. I think I'll try that later today :)

@camelid
Copy link
Contributor

@camelid camelid commented Jul 15, 2020

@jdm I changed ScriptOrigin.text to be an Rc<DOMString>, but I'm getting this (among other) errors:

error[E0277]: the trait bound `std::rc::Rc<dom::bindings::str::DOMString>: malloc_size_of::MallocSizeOf` is not satisfied
   --> components/script/dom/htmlscriptelement.rs:152:23
    |
152 | #[derive(JSTraceable, MallocSizeOf)]
    |                       ^^^^^^^^^^^^ the trait `malloc_size_of::MallocSizeOf` is not implemented for `std::rc::Rc<dom::bindings::str::DOMString>`
    |
    = note: required by `malloc_size_of::MallocSizeOf::size_of`
    = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

What should I do?

@CYBAI
Copy link
Collaborator

@CYBAI CYBAI commented Jul 15, 2020

In this case, I think you can add use ignore_malloc_size_ofattribute to the text property so that compiler will ignore MallocSizeOf for it.

For example, the external_underlying_source property of ReadableStream 👇

#[dom_struct]
pub struct ReadableStream {
reflector_: Reflector,
#[ignore_malloc_size_of = "SM handles JS values"]
js_stream: Heap<*mut JSObject>,
#[ignore_malloc_size_of = "SM handles JS values"]
js_reader: Heap<*mut JSObject>,
has_reader: Cell<bool>,
#[ignore_malloc_size_of = "Rc is hard"]
external_underlying_source: Option<Rc<ExternalUnderlyingSourceController>>,
}

@camelid
Copy link
Contributor

@camelid camelid commented Jul 15, 2020

Thanks! What reason should I put for the macro?

EDIT: Never mind, just saw the reason in your example :)

@camelid camelid mentioned this issue Jul 15, 2020
4 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.