Implement GridTemplateAreas with reference counting #19465

CYBAI · 2017-12-02T18:22:43Z

Use Arc to implement refcounting for GridTemplateAreas
r? emilio

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Improve GridTemplateAreas with reference counting #19428
These changes do not require tests

This change is

highfive · 2017-12-02T18:22:46Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/gecko.mako.rs, components/style/values/specified/position.rs, components/style/properties/shorthand/position.mako.rs
@canaltinova: components/style/properties/gecko.mako.rs, components/style/values/specified/position.rs, components/style/properties/shorthand/position.mako.rs
@emilio: components/style/properties/gecko.mako.rs, components/style/values/specified/position.rs, components/style/properties/shorthand/position.mako.rs

highfive · 2017-12-02T18:22:48Z

Warning

These commits modify style code, but no tests are modified. Please consider adding a test!

tigercosmos · 2017-12-02T18:36:27Z

components/style/values/specified/position.rs

+}
+
+impl MallocSizeOf for TemplateAreasArc {
+    /// FIXIME: (cybai) NEED TO FIGURE OUT HOW TO IMPLEMENT


FIXIME -> FIXME

Oh, thanks :P

tigercosmos · 2017-12-03T05:57:59Z

@CYBAI

test specified_values::size_of_specified_values ... FAILED

failures:

---- specified_values::size_of_specified_values stdout ----

	thread 'specified_values::size_of_specified_values' panicked at 'Your changes have decreased the size of grid_template_areas SpecifiedValue to 8. Good work! The threshold is currently 24. Please consider removing `boxed="True"` from this longhand.', /home/travis/build/servo/servo/tests/unit/stylo/specified_values.rs:48:8

note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

stack backtrace:

   0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace

             at /checkout/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49

   1: std::sys_common::backtrace::_print

             at /checkout/src/libstd/sys_common/backtrace.rs:71

   2: std::panicking::default_hook::{{closure}}

             at /checkout/src/libstd/sys_common/backtrace.rs:60

             at /checkout/src/libstd/panicking.rs:381

   3: std::panicking::default_hook

             at /checkout/src/libstd/panicking.rs:391

   4: std::panicking::rust_panic_with_hook

             at /checkout/src/libstd/panicking.rs:611

   5: std::panicking::begin_panic

             at /checkout/src/libstd/panicking.rs:572

   6: std::panicking::begin_panic_fmt

             at /checkout/src/libstd/panicking.rs:522

   7: stylo_tests::specified_values::size_of_specified_values

             at ./specified_values.rs:48

   8: <F as test::FnBox<T>>::call_box

             at /checkout/src/libtest/lib.rs:1480

             at /checkout/src/libcore/ops/function.rs:223

             at /checkout/src/libtest/lib.rs:141

   9: __rust_maybe_catch_panic

             at /checkout/src/libpanic_unwind/lib.rs:99

failures:

    specified_values::size_of_specified_values

test result: FAILED. 767 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '--lib'

The command "./mach test-stylo" exited with 101.

CYBAI · 2017-12-03T06:08:38Z

@tigercosmos
Yes, I know the failure.
I'll check it later but I guess it's related to MallocSizeOf trait which I implemented it with zero.
Thanks.

emilio

The test-failure is legit and means that with your changes TemplateAreas is no longer big (it's just a pointer now), so you should be able to just remove boxed=True from it.

emilio · 2017-12-03T12:13:00Z

components/style/values/specified/position.rs

+/// Arc type for `Arc<TemplateAreas>`
+pub struct TemplateAreasArc(pub Arc<TemplateAreas>);
+
+impl Parse for TemplateAreasArc {


We should be able to remove Parse for TemplateAreas now, right? Alternatively this method could be:

TemplateAreasArc(Arc::new(TemplateAreas::parse(context, input)?))

I remember I impl this trait due to getting errors about lack of this trait.
I'll try to remove it to see if it's necessary.
Or, I'll try to update the method as returning a TemplateAreasArc.
Thanks.

Hmm... yeah, I'll get error messages like following

error[E0599]: no function or associated item named `parse` found for type `values::Either<values::specified::position::TemplateAreasArc, values::None_>` in the current scope --> /servo/target/geckolib/debug/build/style-abfaa561961fdc53/out/properties.rs:40333:13 | 40333 | specified::GridTemplateAreas::parse(context, input) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: the method `parse` exists but the following trait bounds were not satisfied: `values::specified::position::TemplateAreasArc : parser::Parse` = help: items from traits can only be used if the trait is implemented and in scope = note: the following traits define an item `parse`, perhaps you need to implement one of them: candidate #1: `parser::Parse` candidate #2: `bitflags::<unnamed>::str::StrExt` candidate #3: `style_traits::Separator` error: aborting due to previous error

Let me try to update the method body like what you wrote.

emilio · 2017-12-03T12:13:28Z

components/style/values/specified/position.rs

+}
+
+impl MallocSizeOf for TemplateAreasArc {
+    /// FIXME: (cybai) NEED TO FIGURE OUT HOW TO IMPLEMENT


nit: The comment doesn't really need to be uppercase :)

Haha, Let me updated this into normal cases :P

emilio · 2017-12-03T12:14:06Z

components/style/values/specified/position.rs

+    /// FIXME: (cybai) NEED TO FIGURE OUT HOW TO IMPLEMENT
+    /// MallocConditionalSizeOf for this Arc
+    fn size_of(&self, _ops: &mut MallocSizeOfOps) -> usize {
+        0


@nnethercote, what's the best way to measure this? For now measuring as 0 may be fine, since this is pretty rare, but in case it comes up again...

CYBAI · 2017-12-03T15:17:50Z

@emilio Yeah, I just tried to remove the boxed=True and the test passed locally!
Thanks a lot and I know why we need boxed now!

nnethercote · 2017-12-03T22:01:29Z

About measuring TemplateAreasArc -- can't you just defer to the implementation of MallocConditionalSizeOf for the inner Arc<>?

CYBAI · 2017-12-04T14:50:34Z

@nnethercote I just tried to use conditional_size_of as the method body of impl MallocSizeOf for TemplateAreasArc and I just updated it in this PR. Please help me review it again. Thanks!

cc @emilio

nnethercote · 2017-12-05T02:13:14Z

components/style/values/specified/position.rs

+#[cfg(feature = "gecko")]
+impl MallocSizeOf for TemplateAreasArc {
+    fn size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
+        self.0.conditional_size_of(ops)


This isn't quite right. TemplateAreasArc is basically the same as Arc, so it should implement the same traits: one or more of MallocUnconditionalShallowSizeOf, MallocUnconditionalSizeOf, MallocConditionalShallowSizeOf, MallocConditionalSizeOf. It should not implement MallocSizeOf or MallocShallowSizeOf. See components/malloc_size_of/lib.rs for more details.

Having said that: does TemplateAreasArc need to be a newtype? Could it just be a typedef for Arc<TemplateAreas>? Maybe you need a newtype so you can define additional methods?

TemplateAreasArc

Right, it needs to be a newtype so it can implement ToCss and such.

Though arguably that should be generically implemented... ToComputedValue is trickier though.

@nnethercote Due to enum PropertyDeclaration, we must impl MallocSizeOf for TemplateAreasArc so I cannot only impl MallocConditionalSizeOf for it.
How do you think about it?

cc @emilio

Is #[derive(MallocSizeOf)] involved? It works well for tree-like structures, but less well for graph-like structures. One possibility is to change the relevant MallocSizeOf impl from a derived one to a manually written one that does something appropriate (and different) for the Arc. Another possibility is to keep the derive impl and use the ignore_malloc_size_of attribute on the relevant Arc field.

Yes, this code block is involved #[derive(MallocSizeOf)].

servo/components/style/properties/properties.mako.rs

Lines 1404 to 1408 in 0fa605d

/// Servo's representation for a property declaration.

#[cfg_attr(feature = "gecko", derive(MallocSizeOf))]

#[derive(Clone, PartialEq)]

pub enum PropertyDeclaration {

% for property in data.longhands:

I think adding another checking into mako for only this case might not be good.
I'll try to use ignore_malloc_size_of attribute for it, thanks.

bors-servo · 2017-12-16T22:40:52Z

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

CYBAI · 2017-12-24T14:28:43Z

@nnethercote @emilio I just updated the PR with keeping the impl of MallocSizeOf and use ignore_malloc_size_of attribute!
Please help me to review it again, thanks! 😁

CYBAI · 2017-12-24T14:37:03Z

Travis build failed with #19634

CYBAI · 2017-12-25T02:48:54Z

Rebased and fixed the build error.

CYBAI · 2018-01-04T11:46:25Z

r? @nnethercote and @emilio
Ping for review, thanks!

bors-servo · 2018-01-23T18:57:33Z

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

CYBAI · 2018-01-29T14:19:58Z

Ping for review from @nnethercote and @emilio
Also, the S-needs-rebase label is outdated.

nnethercote · 2018-01-31T00:20:02Z

components/style/values/specified/position.rs

@@ -624,6 +625,23 @@ impl Parse for TemplateAreas {

 trivial_to_computed_value!(TemplateAreas);

+#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)]
+/// Arc type for `Arc<TemplateAreas>`


I'd put the comment above the derive line.

nnethercote

Apologies for the long delay, I was on PTO.

The memory measurements parts of this patch look fine to me. Emilio will need to approve the rest of it, though.

nnethercote · 2018-01-31T00:21:49Z

r? @emilio

emilio · 2018-01-31T13:27:59Z

Looks good, thanks!

@bors-servo r+

bors-servo · 2018-01-31T13:28:00Z

📌 Commit 9004fff has been approved by emilio

Implement GridTemplateAreas with reference counting Use `Arc` to implement refcounting for `GridTemplateAreas` r? emilio --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #19428 - [x] These changes do not require tests  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19465)

bors-servo · 2018-01-31T13:28:09Z

⌛ Testing commit 9004fff with merge 12a5966...

bors-servo · 2018-01-31T14:21:05Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: emilio
Pushing 12a5966 to master...

highfive assigned emilio Dec 2, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Dec 2, 2017

tigercosmos reviewed Dec 2, 2017

View reviewed changes

CYBAI force-pushed the refcount-template-area branch from 507e5f4 to 4c8778f Compare December 2, 2017 18:44

emilio reviewed Dec 3, 2017

View reviewed changes

CYBAI force-pushed the refcount-template-area branch 3 times, most recently from 0032052 to 027f37e Compare December 3, 2017 17:29

CYBAI force-pushed the refcount-template-area branch from 027f37e to e76d8bd Compare December 4, 2017 14:48

nnethercote reviewed Dec 5, 2017

View reviewed changes

highfive added the S-needs-rebase There are merge conflict errors. label Dec 16, 2017

CYBAI force-pushed the refcount-template-area branch 2 times, most recently from c013964 to b361e79 Compare December 17, 2017 14:46

jdm removed the S-needs-rebase There are merge conflict errors. label Dec 18, 2017

CYBAI force-pushed the refcount-template-area branch 2 times, most recently from c6dac38 to bb7237e Compare December 24, 2017 13:57

CYBAI force-pushed the refcount-template-area branch from bb7237e to 6a23362 Compare December 25, 2017 01:41

highfive unassigned emilio Jan 4, 2018

highfive assigned nnethercote Jan 4, 2018

jdm assigned emilio and unassigned nnethercote Jan 4, 2018

highfive added the S-needs-rebase There are merge conflict errors. label Jan 23, 2018

CYBAI force-pushed the refcount-template-area branch 2 times, most recently from 0c79fd8 to 6c48285 Compare January 25, 2018 02:31

nnethercote reviewed Jan 31, 2018

View reviewed changes

Implement GridTemplateAreas with reference counting

9004fff

CYBAI force-pushed the refcount-template-area branch from 6c48285 to 9004fff Compare January 31, 2018 01:53

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Jan 31, 2018

bors-servo merged commit 9004fff into servo:master Jan 31, 2018

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 31, 2018

CYBAI deleted the refcount-template-area branch January 31, 2018 15:34

	/// Servo's representation for a property declaration.
	#[cfg_attr(feature = "gecko", derive(MallocSizeOf))]
	#[derive(Clone, PartialEq)]
	pub enum PropertyDeclaration {
	% for property in data.longhands:

Implement GridTemplateAreas with reference counting #19465

Implement GridTemplateAreas with reference counting #19465

Conversation

CYBAI commented Dec 2, 2017 • edited by emilio

highfive commented Dec 2, 2017

highfive commented Dec 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigercosmos commented Dec 3, 2017

CYBAI commented Dec 3, 2017

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CYBAI Dec 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CYBAI commented Dec 3, 2017

nnethercote commented Dec 3, 2017

CYBAI commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-servo commented Dec 16, 2017

CYBAI commented Dec 24, 2017 • edited

CYBAI commented Dec 24, 2017

CYBAI commented Dec 25, 2017

CYBAI commented Jan 4, 2018 • edited

bors-servo commented Jan 23, 2018

CYBAI commented Jan 29, 2018

Choose a reason for hiding this comment

nnethercote left a comment

Choose a reason for hiding this comment

nnethercote commented Jan 31, 2018

emilio commented Jan 31, 2018

bors-servo commented Jan 31, 2018

bors-servo commented Jan 31, 2018

bors-servo commented Jan 31, 2018

CYBAI commented Dec 2, 2017 •

edited by emilio

CYBAI Dec 3, 2017 •

edited

CYBAI commented Dec 24, 2017 •

edited

CYBAI commented Jan 4, 2018 •

edited