Closed Bug 1480649 Opened 2 years ago Closed 2 years ago

Use union to store different shape-like types in StyleShapeSource

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: boris, Assigned: boris)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
Summary: Use RefPtr → Use RefPtr to store mShapeOutside, instead of object
We store the computed value of <shape-outside> as mozilla::StyleShapeSource object [1], and StyleShapeSource includes some UniquePtrs and some types. In Bug 1186329, I would like to introduce more types (e.g. SVG Path, Ray function) into StyleShapeSource, so it's better to use a smart pointer to store shape-outside, instead of the object.

Therefore, we can make sure any future update on StyleShapeSource wouldn't increase the size of nsStyleDisplay, which has a upper limit of 504 bytes [2].

[1] https://searchfox.org/mozilla-central/rev/e52cd92858800a69b74cb97d26d9bdb960d611ca/layout/style/nsStyleStruct.h#2224
[2] https://searchfox.org/mozilla-central/rev/e52cd92858800a69b74cb97d26d9bdb960d611ca/layout/style/nsStyleStruct.cpp#51-54
Assignee: nobody → boris.chiou
Blocks: motion-1
Status: NEW → ASSIGNED
Priority: -- → P3
Summary: Use RefPtr to store mShapeOutside, instead of object → Use RefPtr to store mShapeOutside
Or an alternative way is to use `union` for different shape types.
Why RefPtr instead of UniquePtr btw? Though if the types are exclusive maybe an union is ok.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)
> Why RefPtr instead of UniquePtr btw? Though if the types are exclusive maybe
> an union is ok.

At first, I also want to use UniquePtr for mShapeOutside, e.g.

UniquePtr<mozilla::StyleShapeSource> mShapeOutside;

However, sometimes we need to copy the members of nsStyleDisplay (e.g. in the copy constructor of nsStyleDisplay [1]), and it seems if we use UniquePtr, we have to copy the StyleShapeSource object, instead of just adding the reference count. I'm not sure if it is ok to "move" the object from old nsStyleDisplay to the new one, so maybe using RefPtr is better.

[1] https://searchfox.org/mozilla-central/rev/e52cd92858800a69b74cb97d26d9bdb960d611ca/layout/style/nsStyleStruct.cpp#3611


In StyleShapeSource, I could use union to store its shape-like types, e.g.

class StyleShapeSource {
...
private:
  union {
    UniquePtr<StyleBasicShape> mBasicShape;
    UniquePtr<nsStyleImage> mShapeImage;
    UniquePtr<StyleSVGPath> mSVGPath; // This will be added in a different bug for offset-path and clip-path. 
    UniquePtr<StyleRay> mRay; // This will be added for <offset-path>
  };
};
(In reply to Boris Chiou [:boris] from comment #4)
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #3)
> > Why RefPtr instead of UniquePtr btw? Though if the types are exclusive maybe
> > an union is ok.
> 
> At first, I also want to use UniquePtr for mShapeOutside, e.g.
> 
> UniquePtr<mozilla::StyleShapeSource> mShapeOutside;
> 
> However, sometimes we need to copy the members of nsStyleDisplay (e.g. in
> the copy constructor of nsStyleDisplay [1]), and it seems if we use
> UniquePtr, we have to copy the StyleShapeSource object, instead of just
> adding the reference count. I'm not sure if it is ok to "move" the object
> from old nsStyleDisplay to the new one, so maybe using RefPtr is better.

No, it's not ok to move it.

But copying an nsStyleDisplay with non-default members should be really really rare, to the point where the copy is not really problematic I'd think, so as long as the default constructor has a null StyleShapeSource it should be fine.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)
> No, it's not ok to move it.
> 
> But copying an nsStyleDisplay with non-default members should be really
> really rare, to the point where the copy is not really problematic I'd
> think, so as long as the default constructor has a null StyleShapeSource it
> should be fine.

OK, I see. Let's use UniquePtr for mShapeOutside. i.e. `UniquePtr<mozilla::StyleShapeSource> mShapeOutside`.
Summary: Use RefPtr to store mShapeOutside → Use smart pointer to store mShapeOutside
1. We will add more shape-like types in the future, so it's better to
   use union to reduce the memory usage.
2. Those shape-like types are mutual exclusive, so we could use union to
   wrap them.
Summary: Use smart pointer to store mShapeOutside → Use union to store different shape-like types for StyleShapeSource
Summary: Use union to store different shape-like types for StyleShapeSource → Use union to store different shape-like types in StyleShapeSource
(In reply to Boris Chiou [:boris] from comment #6)
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #5)
> > No, it's not ok to move it.
> > 
> > But copying an nsStyleDisplay with non-default members should be really
> > really rare, to the point where the copy is not really problematic I'd
> > think, so as long as the default constructor has a null StyleShapeSource it
> > should be fine.
> 
> OK, I see. Let's use UniquePtr for mShapeOutside. i.e.
> `UniquePtr<mozilla::StyleShapeSource> mShapeOutside`.

It seems there are a lot of updates to use UniquePtr for both mShapeOutside and mClipPath, so it might be better to review the union patch first.
Comment on attachment 8997603 [details]
Bug 1480649 - Use union to wrap different shape-like types.

Xidorn Quan [:xidorn] UTC+10 has approved the revision.

https://phabricator.services.mozilla.com/D2746
Attachment #8997603 - Flags: review+
Pushed by boris.chiou@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/924956d87045
Use union to wrap different shape-like types. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/924956d87045
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.