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

Make to_shmem Gecko-only #26770

Closed
wants to merge 1 commit into from
Closed

Make to_shmem Gecko-only #26770

wants to merge 1 commit into from

Conversation

@SimonSapin
Copy link
Member

SimonSapin commented Jun 3, 2020

As suggested in #26713 (comment)

@highfive
Copy link

highfive commented Jun 3, 2020

Heads up! This PR modifies the following files:

  • @emilio: components/style/values/generics/flex.rs, components/style/stylesheets/style_rule.rs, components/style/gecko/url.rs, components/style/gecko_string_cache/mod.rs, components/style/stylesheets/origin.rs and 85 more
@highfive
Copy link

highfive commented Jun 3, 2020

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!
@jdm
jdm approved these changes Jun 3, 2020
@@ -25,17 +25,9 @@ macro_rules! ns {

/// A Gecko namespace is just a wrapped atom.
#[derive(
Clone,

This comment has been minimized.

Copy link
@jdm

jdm Jun 3, 2020

Member

I don't think we need to add feature checks to any of the style/gecko/ or style/gecko_string_cache files.

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Jun 3, 2020

Author Member

Good point. Amended.

As suggested in #26713 (comment)
@jdm
Copy link
Member

jdm commented Jun 3, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2020

📌 Commit caf4cc6 has been approved by jdm

@highfive highfive assigned jdm and unassigned asajeffrey Jun 3, 2020
@emilio
Copy link
Member

emilio commented Jun 3, 2020

@bors-servo r-

Can we discuss this for a second? This is going to make syncs even more painful and if I understand correctly this affects llvm IR but not generated code, right?

@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2020

The latest upstream changes (presumably #26775) made this pull request unmergeable. Please resolve the merge conflicts.

@SimonSapin
Copy link
Member Author

SimonSapin commented Jun 4, 2020

Correct, this is about reducing compile times. I would expect the linker to remove functions that are never called. (Maybe even without LTO? IIRC with --gc-sections is the default in rustc.)

I hadn’t considered the indirect impact on sync as you explained it on Matrix (mozilla-central contributors accidentally breaking the Servo configuration more often as it diverges more).

to_shmem_slice_ptr does show up in #26713 (comment) but with only 0.4% of IR lines in libscript. This is probably not worth the downside, so closing.

@SimonSapin SimonSapin closed this Jun 4, 2020
@SimonSapin SimonSapin deleted the shmem branch Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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