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

style: Allow sharing styles across elements with presentational hints. #17063

Merged
merged 8 commits into from May 30, 2017

Conversation

Projects
None yet
5 participants
@emilio
Copy link
Member

commented May 27, 2017

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented May 27, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/sharing/mod.rs, components/style/rule_tree/mod.rs, components/style/context.rs, components/style/matching.rs, components/style/traversal.rs and 2 more
@highfive

This comment has been minimized.

Copy link

commented May 27, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify style code, but no tests are modified. Please consider adding a test!
@emilio

This comment has been minimized.

Copy link
Member Author

commented May 27, 2017

@highfive highfive assigned bholley and unassigned glennw May 27, 2017

@emilio emilio force-pushed the emilio:pres-hints-sharing branch from e784d37 to 6b402d1 May 29, 2017

@emilio

This comment has been minimized.

Copy link
Member Author

commented May 29, 2017

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97726f83bbda0c08ed821d2bd22a9e0dd71412cb&selectedJob=102756194

(Note that the jpeg test failure is because the try run is on top of my patches for bug 1357583, which introduce that who knows why)

@bholley
Copy link
Contributor

left a comment

So, I really appreciate all the awesome work here, but I'm going to finally bite the bullet and complain about the megapatches because they're driving me crazy. :-)

In general I would ask that you you do the following things, at least for large patches that you want me to review:
(1) Don't include gratuitous reformatting or whitespace change along with other non-trivial changes.
(2) Split up your commits in a way that makes larger change more incrementally.
(3) Post the patches themselves to bugzilla, not just a link to a github PR.

I know this takes more time, but the current approach has the following disadvantages:
(1) I can't review your patches without blocking out a large portion of time, because there's no way for me to track what I've reviewed and what I haven't. This leads to review delays.
(2) The patches take me much longer to review than they otherwise would, which costs me time. The review is also lower quality, because there are too many disjoint things going on, and so it's harder for me to engage with the patch at a precise level and hard not to just hand-wave it.
(3) If I want to r- anything, I have to review the entire patch again.

I've been hesitant to complain about this because you're doing such great work, and I know you almost always get it right. But reviewing your code is costing me way more time than it could, and time is something I'm very short on right now. :-)

/// a good reason to.
/// The element being processed. Currently we use an OpaqueNode since we
/// only use this for identity checks, but we could use SendElement if there
/// were a good reason to.

This comment has been minimized.

Copy link
@bholley

bholley May 29, 2017

Contributor

Can you please stop adding gratuitous whitespace reformats to your patches? It makes them much harder to review, which means they take longer, which means it takes longer for me to find time to review them, which ends up as review delay. I don't think either of us want that. :-)

/// revalidation selectors.
pub revalidation_match_results: Option<BitVec>,
/// Lazy cache of the stuff we use to share style, in case we tried and
/// failed.

This comment has been minimized.

Copy link
@bholley

bholley May 29, 2017

Contributor

Change this comment to:

"When we evaluate whether two elements may share style, we have to compute various values and compare them. We cache this information lazily here so that it's only computed once per element.

/// `StyleSharingCandidate` (indeed we could try to merge those, though
/// conceptually we may still want them to be separate).
#[derive(Debug)]
pub struct CachedStyleSharingData {

This comment has been minimized.

Copy link
@bholley

bholley May 29, 2017

Contributor

Let's call this StyleSharingCriteria.

This comment has been minimized.

Copy link
@emilio

emilio May 29, 2017

Author Member

I don't love "criteria", I don't think it says much, this is just a bunch of data we cache with the purpose of style sharing. But fine, I guess.

/// because it depends on the `style_sharing_candidate_cache` having only
/// live nodes in it, and we have no way to guarantee that at the type
/// system level yet.
pub unsafe fn share_style_if_possible(

This comment has been minimized.

Copy link
@bholley

bholley May 29, 2017

Contributor

This comment and the associated unsafety are bogus now that we we stored this all in ScopedTLS. Feel free to fix.

if self.class_list.is_none() {
let mut ret = SmallVec::new();
element.each_class(|c| ret.push(c.clone()));
self.class_list = Some(ret);

This comment has been minimized.

Copy link
@bholley

bholley May 29, 2017

Contributor

Can we sort this list please? Otherwise we may falsely reject candidates based on different ordering.

This comment has been minimized.

Copy link
@emilio

emilio May 29, 2017

Author Member

I think that's something we may want to land/measure separately.

This comment has been minimized.

Copy link
@bholley

bholley May 29, 2017

Contributor

How come? Sorting a SmallVec of a few atoms is going to be super-cheap...

/// The cached result of matching this entry against the revalidation selectors.
revalidation_match_results: Option<BitVec>,
/// The cached data we store to avoid recomputing too much stuff.
cache: CachedStyleSharingData,

This comment has been minimized.

Copy link
@bholley

bholley May 29, 2017

Contributor

Along with the rename of the struct suggested elsewhere, we should call this (and other names) |criteria| or |sharing_criteria|.

/// A struct we use as a simple cache of stuff we use to test against different
/// candidates.
#[derive(Debug)]
pub struct ElementSharingChecker<E: TElement> {

This comment has been minimized.

Copy link
@bholley

bholley May 29, 2017

Contributor

I'm not really sold on the need for this class. It really seems to me like it's just a bunch of extra boilerplate and indirection, and that we'd be better off by just passing around the element and cache directly.

This comment has been minimized.

Copy link
@emilio

emilio May 29, 2017

Author Member

I think it's nice because it allows you to keep the sharing flags stuff in a well-known comment, but I need to rewrite all this anyway, so we'll see :)

// If we know the element had no pres hints from the relations, we can
// just do as-if we had computed it, regardless of whether we actually
// did.
if !relations.intersects(AFFECTED_BY_PRESENTATIONAL_HINTS) {

This comment has been minimized.

Copy link
@bholley

bholley May 29, 2017

Contributor

This seems like a hack. How about eliminating AFFECTED_BY_PRESENTATIONAL_HINTS altogether, and instead having push_applicable_declarations store the synthesis result directly in the StyleSharingCriteria hanging off the CurrentElementInfo?

This comment has been minimized.

Copy link
@bholley

bholley May 30, 2017

Contributor

I still want to see this AFFECTED_BY_PRESENTATIONAL_HINTS business fixed before landing. Otherwise we synthesize twice where we only synthesized once before.

@@ -228,11 +431,6 @@ impl<E: TElement> StyleSharingCandidateCache<E> {
return StyleSharingResult::CannotShare
}

if element.get_id().is_some() {

This comment has been minimized.

Copy link
@bholley

bholley May 29, 2017

Contributor

Wait what? Where did this come from? This seems like something out of Boris' patch.

This comment has been minimized.

Copy link
@emilio

emilio May 29, 2017

Author Member

This should've landed when Boris landed the element.get_id() != candidate.get_id() changes not long ago.

@emilio

This comment has been minimized.

Copy link
Member Author

commented May 29, 2017

Re. the mega-patches, I'd say that it's fair, but given how long does it take a patch to land, I usually prefer to just stick most related changes together. And re. everything in one commit, that was just that it took less work, and I'm also relatively short on time.

But fair, landing times should be better now, so I'll split this :)

@bholley

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

but given how long does it take a patch to land, I usually prefer to just stick most related changes together.

You can still push an arbitrary number of changesets, even from single bugs, in the same PR.

And re. everything in one commit, that was just that it took less work, and I'm also relatively
short on time.

I know, which is why I've been keeping quiet about it, but this review took me more than 40 minutes to do (ignoring the time I spent complaining about the megapatches), and I still don't feel like I did a very good job. I could have reviewed it in a quarter of the time if it was split up.

@bholley

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

I could do ValidationInfo or ValidationData instead of criteria.

emilio added some commits May 29, 2017

style: Allow ApplicableDeclarationBlocks to be compared.
We'll need this to cache pres hints, which generate these.
style: Also cache the class list in the CurrentElementInfo.
This patch also removes all notion of style sharing from matching.rs, which is
nice.

@emilio emilio force-pushed the emilio:pres-hints-sharing branch from 6b402d1 to 0adc02a May 29, 2017

@emilio

This comment has been minimized.

Copy link
Member Author

commented May 29, 2017

Rewritten, just read the comments re. ValidationInfo. Need to run now, happy to rename it in a fixup commit. Feel free to review assuming that.

@bholley
Copy link
Contributor

left a comment

r=me with those things fixed. Thanks for splitting things up! Next time feel free to combine the PartialEq impls into the commits that use them, and please submit the patches to bugzilla, because github's tracking of revisions for multiple patches is pretty broken. :-)

@@ -61,6 +61,12 @@ pub enum StyleSource {
Declarations(Arc<Locked<PropertyDeclarationBlock>>),
}

impl PartialEq for StyleSource {

This comment has been minimized.

Copy link
@bholley

bholley May 30, 2017

Contributor

FWIW: While I appreciate the effort, separate patches for things like a PartialEq implementation aren't necessary. :-)

pub struct CachedStyleSharingData {
/// The class list of this element.
///
/// TODO(emilio): See if it's worth to sort them, or doing something else in

This comment has been minimized.

Copy link
@bholley

bholley May 30, 2017

Contributor

I really think it is worth sorting. I'd prefer to just add that as a one-line change to this patch, but if you really want to keep it separate, can you at least file a followup so we don't forget?

&mut self,
data);

// FIXME(emilio): Do this in a cleaner way.

This comment has been minimized.

Copy link
@bholley

bholley May 30, 2017

Contributor

Can you elaborate on this? Maybe you should just call take()?


/// Move the cached data to a new instance, and return it.
pub fn take(&mut self) -> Self {
Self {

This comment has been minimized.

Copy link
@bholley

bholley May 30, 2017

Contributor

It seems more robust to implement this in terms of a swap with a default-constructed instance. That way we don't miss members that get added later.

This comment has been minimized.

Copy link
@emilio

emilio May 30, 2017

Author Member

How can this miss any member, exactly?

@@ -50,6 +51,23 @@ pub struct CachedStyleSharingData {
}

impl CachedStyleSharingData {
/// Trivially construct an empty `CachedStyleSharingData` with nothing on
/// it.

This comment has been minimized.

Copy link
@bholley

bholley May 30, 2017

Contributor

I'm generally for making trivial constructors impl Default instead, though I won't insist.

// If we know the element had no pres hints from the relations, we can
// just do as-if we had computed it, regardless of whether we actually
// did.
if !relations.intersects(AFFECTED_BY_PRESENTATIONAL_HINTS) {

This comment has been minimized.

Copy link
@bholley

bholley May 30, 2017

Contributor

I still want to see this AFFECTED_BY_PRESENTATIONAL_HINTS business fixed before landing. Otherwise we synthesize twice where we only synthesized once before.

@bholley

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

Oh, and the most important review comment seems to have been swallowed by github: Please fix things up to cache the presentational hints synthesized in the stylist and avoid re-synthesizing them for the cache.

@bors-servo delegate+

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

✌️ @emilio can now approve this pull request

s/CachedStyleSharingData/ValidationData.
I still think CachedStyleSharingData should be the name, but not going to fight
over it.
@emilio

This comment has been minimized.

Copy link
Member Author

commented May 30, 2017

I left this on IRC, but just so it's not missed:

11:09 <emilio> bholley: I looked into the caching pres hints thing... I don't really like the result, complexity wise. I've been thinking on replacing most of the arguments to push_applicable_declarations with something else that abstracts the getting animation rules, style attribute, etc, so maybe that allows to do it in a clean way
11:10 <emilio> bholley: anyway, I don't really want to complicate code on purpose for something that we a) don't have perf numbers for, and b) is already really rare (most elements don't have any pres hints)

@bors-servo r=bholley

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

📌 Commit 7db2776 has been approved by bholley

@emilio

This comment has been minimized.

Copy link
Member Author

commented May 30, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

⌛️ Testing commit 7db2776 with merge 38a6a3b...

bors-servo added a commit that referenced this pull request May 30, 2017

Auto merge of #17063 - emilio:pres-hints-sharing, r=bholley
style: Allow sharing styles across elements with presentational hints.

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17063)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

@bors-servo bors-servo merged commit 7db2776 into servo:master May 30, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.