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

Efficient storage of CSS #1923

Closed
wants to merge 18 commits into from
Closed

Conversation

ssjkamei
Copy link

Large tables require huge memory to store CSS.

We believe this problem can be solved by streamlining the memory storage of Cascaded styles.

The fix is an attempt to compress what is loaded the first time, but I think it contains a problem that requires deepcopy and is not very fast.

The same kind of fix for ComputedStyle compressed the relevant processing part from 178MB to about 13MB for a table with about 3000 rows.
Because ComputedStyle shares cascaded styles, it must be saved once without the element, parent style, and root style. What I tried at this point was to create and use a separate class for relationships.
I think a complete modification would also require sharing Box and Page styles. (I'm also wondering if it would be better to change the parenting scheme for the Class itself).

I need to handle a table with about 20,000 rows,
Currently, I am assuming 4-6 GB will be used. I think I can get it down to under 500MB once it is fully modified.

Thanks for reading.

@ssjkamei
Copy link
Author

ssjkamei commented Aug 2, 2023

I would like to know the following.

  • Is the fix acceptable?
  • Is there a better way than sharing the same CSS?

Please let me know if I made a mistake, such as I should raise the issue first.

@liZe
Copy link
Member

liZe commented Aug 2, 2023

I would like to know the following.

  • Is the fix acceptable?
  • Is there a better way than sharing the same CSS?

Please let me know if I made a mistake, such as I should raise the issue first.

Hi!

Sorry for the delay, we’re currently very busy working on other projects.

Thanks a lot for your pull request, we’ll take the time to review it and get back to you as soon as possible 💜.

@liZe
Copy link
Member

liZe commented Aug 19, 2023

Hi!

Thanks a lot for the pull request. I’ve finally taken the time to review your code, and it’s a pretty good idea. You’ll probably love to read this article that takes this idea even a bit further!

There are many problems that prevent your idea from being simple to implement. Here are the 2 principal ones I know about:

  • Style values contain mutable elements. That’s a shame, because without this it would be easier to create real hashes used as index keys, containing both keys and values. One mutable element is page specificity, and it’s easy to fix. Another one is the content list (used to store values of content attributes), and it’s harder to fix because we do dirty things like this one.

  • Styles are modified, I suppose that’s why you need copies. An example is set_trasparent_border. That’s something we should fix (fixing bugs like Collapsed borders not drawn for tables in margin boxes #1278) before getting something clean I think.

So it’s a bit too early to get a clean way to implement your idea in a clean way. Working on the first item is possible and will help to at least get a simpler cache structure. The other one is more complicated.

@liZe liZe marked this pull request as draft August 20, 2023 12:09
@ssjkamei
Copy link
Author

ssjkamei commented Aug 21, 2023

Thank you for your review.

_cascaded_styles is a simple fix because it is disposable to create _computed_styles.
For example, it was difficult to pass the test in the area you noted where the parent style needed to be changed and copied after the _computed_styles were saved. (In fact, the last time we tried it, one case didn't pass and we couldn't figure it out.)

However, it is worth doing first just the sharing part of saving and retrieving the ComputedStyle, without considering the sharing of each acquired box (e.g., efficient sharing of AnonymousStyle).

Specifically, I'm thinking of splitting the ComputedStyle attribute into the following Class to separate the relationship.
class ComputedStyle Attr: Element, pseudo_type, specified, Cascaded(Indexed), is_root_element, base_url
new class StyleRelationship Attr: Element, pseudo_type, Parent Element, Style(Indexed), rootStyle(Indexed)

There is a possibility that the parent style call may require some changes in code other than css/__init__.py, so we will try again to see if we can avoid this one.

If the challenge does not result in clean code, I will give it some time until I can come up with a good solution.

@ssjkamei
Copy link
Author

Sorry, I did not read the article.

I don't know if I can do it, but I will try the following first.

In Quantum CSS, we gather up all of those weird selectors and check whether they apply to the DOM node. Then we store the answers as ones and zeros. If the two elements have the same ones and zeros, we know they definitely match.

I had first thought about sharing by structure, but not being familiar with css, I was looking for a way to reflect the current code without changing it as much as possible.
I would appreciate it if you could point out if my interpretation is wrong.

@liZe
Copy link
Member

liZe commented Aug 21, 2023

_cascaded_styles is a simple fix because it is disposable to create _computed_styles. For example, it was difficult to pass the test in the area you noted where the parent style needed to be changed and copied after the _computed_styles were saved. (In fact, the last time we tried it, one case didn't pass and we couldn't figure it out.)

However, it is worth doing first just the sharing part of saving and retrieving the ComputedStyle, without considering the sharing of each acquired box (e.g., efficient sharing of AnonymousStyle).

Specifically, I'm thinking of splitting the ComputedStyle attribute into the following Class to separate the relationship. class ComputedStyle Attr: Element, pseudo_type, specified, Cascaded(Indexed), is_root_element, base_url new class StyleRelationship Attr: Element, pseudo_type, Parent Element, Style(Indexed), rootStyle(Indexed)

👍

Sorry, I did not read the article.

That’s just an article I find very inspiring on the topic, but there’s no need to follow it for your code! Maybe one day, but for now we’re far from what Firefox does.

Thanks a lot!

@ssjkamei
Copy link
Author

ssjkamei commented Aug 28, 2023

Thanks for the continued support.

I have also tried modifying the ComputedStyle, but I am getting errors in the following three cases. I would appreciate if someone could give me some advice.

The simple calculation of each code seems to be working, but I think it is wrong that the specified px is automatically calculated in the layout/flex.py (flex_layout), but I am struggling to find the correct process.

  • test_flex_align_content
  • test_flex_row_wrap
  • test_flex_row_wrap_reverse

It looks like it could be done nicely with less modifications than I thought.
I ended up not excluding the css recalculation because it seems difficult to save the class/id properties.
I continue to use a hash to summarize them to some extent.
I am hoping for a great recalculation process.

We couldn't decide if we should also share AnonymousStyle, so AnonymousStyle is mostly intact.

Even with the current code, we have been able to reduce the amount of memory significantly, but there are many issues to be addressed. (It seems to be about 1/4 the size in my test environment.)

@ssjkamei
Copy link
Author

I'm sorry. The test case we compared was incorrect.
The reduction is about 23%.

The slow deepcopy is causing the execution time to increase significantly. I will try to improve the code.

Table: 2500rows

mem sec
830MB 120.99
After change 638MB 2,077.84

@ssjkamei
Copy link
Author

Table: 2500rows

mem sec
830MB 120.99
After change 638MB 258.01

@ssjkamei
Copy link
Author

As far as the distribution of memory specifications is concerned, as confirmed by the document.py _render method, the larger the table, the more memory usage will approach 1/3 of the current amount.

If it is possible to compress the amount used by the following process, it may be possible to make it even smaller.

rendering = cls(
            [Page(page_box) for page_box in page_boxes],.
            DocumentMetadata(**get_html_metadata(html)),
            html.url_fetcher, font_config)

A more efficient logic is desired if possible, as some people may not tolerate the slowness.

I can see that the following is occurring in the class ComputedStyle copy() method, and I think I can modify it by placing a dummy element, etc., but I don't understand exactly what you are trying to do. Please continue to advise.

The simple calculation of each code seems to be working, but I think it is wrong that the specified px is automatically calculated in the layout/flex.py (flex_layout), but I am struggling to find the correct process.

@ssjkamei
Copy link
Author

I also tried AnonymousStyle sharing. It still could not be compressed better than the others.
I would like ideas to reduce the number of saves with the current system to reduce execution time. I will give more thought to this.

Table: 2500rows

mem sec
830MB 120.99
After change 545MB 303.45

@ssjkamei
Copy link
Author

ssjkamei commented Aug 31, 2023

Significant memory compression is expected for large tables.
I consider execution time to be an issue.

Table: 13000rows

mem sec
4,051MB 612.67
After change 2,050MB 1,720.80

@ssjkamei
Copy link
Author

For ComputedStyle with the same cascading style index, would it reduce processing if I could mark it as computed, or would that not be very effective since it would have no effect on AnonymousStyles?

I'll try to understand the following process mentioned in this article, which may be necessary, by following the code that is running.

In Quantum CSS, we gather up all of those weird selectors and check whether they apply to the DOM node. elements have the same ones and zeros, we know they definitely match.

@ssjkamei
Copy link
Author

I can see that the following is occurring in the class ComputedStyle copy() method, and I think I can modify it by placing a dummy element, etc., but I don't understand exactly what you are trying to do. Please continue to advise.

The test is now passing, but I'm not very confident in the implementation.

@liZe
Copy link
Member

liZe commented Jan 7, 2024

Thanks for your hard work on this.

I like the overall goal of this pull request, but I have two main problems with it:

  • It adds more complexity to the cascade and the computation of values, that’s already a complex topic.
  • It’s slower. I don’t know the exact reasons, but one detail is important: inheriting Style classes from dict (at least for __getitem__ and __setitem__) gives a impressive speed boost. It’s actually been a nice and painful achievement (partly discussed in Don't copy styles only to change margins, borders and paddings #497) and I think that it’s important for both simplicity and performance.

I'll try to understand the following process mentioned in this article, which may be necessary, by following the code that is running.

The global idea is, for each tag, to list all the matching selectors. If the list of selectors is exactly the same, then we can use the same computed values (and avoid a lot of copies). To be honest, I’m not sure that it can really work this way anymore, as more and more computed values depend on the content of the tag (for example when using attr() or fit-content).

@ssjkamei
Copy link
Author

ssjkamei commented Jan 8, 2024

Thank you!

Sorry, I was not able to respond to your other issue.

I will try to organize it so that it can be corrected in dict.
I am trying to figure out if it is possible to include the sharing process when processing cascade style.

I will try to make some of the processing done in __missing__ of ComputedStyle into a function, and try to flag attributes with computed processing.

@ssjkamei
Copy link
Author

ssjkamei commented Jan 9, 2024

The Speed up initial render (and the cascade) with the style sharing cache section seemed efficient enough in the current code.
If I am not mistaken, it did not seem to store the DOM of the table as efficiently as I would have liked, as it would have to have a value for each parent DOM.

If we can do what is specified in the A sidenote: style struct sharing section, we may be closer to the goal of this branch, so I will try to implement this one.

@liZe
Copy link
Member

liZe commented Mar 17, 2024

Closing for now, please add a comment if you want to share new commits on this topic.

@liZe liZe closed this Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants