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

Improve <SvgIcon> to make it output svg node and optimize performance #23570

Merged
merged 10 commits into from
Mar 23, 2023

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Mar 19, 2023

Before, the Vue <SvgIcon> always outputs DOM nodes like:

<span class="outer-class">
    <svg class="class-name-defined" ...></svg>
</span>

The span is redundant and I guess such layout and the inconsistent class/class-name attributes would cause bugs sooner or later.

This PR makes the <SvgIcon> clear, and it's faster than before, because it doesn't need to parse the whole SVG string.

Before:

image

After:

image

@wxiaoguang wxiaoguang changed the title Improve <SvgIcon> to make it output single node and optimize performance Improve <SvgIcon> to make it output svg node and optimize performance Mar 19, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 19, 2023
@silverwind
Copy link
Member

silverwind commented Mar 19, 2023

This does seem like a lot of extra code and it redoes most of what svg already does. I wonder if it wouldn't be better to have a shared parser function that returns {attributes, innerHTML} which could be used in both. It would lose the fast-path of svg with size 16 and no classes, thought, so that is a downside and probably not worth because of that.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 19, 2023

This does seem like a lot of extra code and it redoes most of what svg already does.

Nope, not extra. Just some comment, and necessary code to handle attributes / "class"+"class-name" correctly.

I wonder if it wouldn't be better to have a shared parser function that returns {attributes, innerHTML} which could be used in both.

They do not do the same thing. The key point is, in your old svg function, the string is returned for default arguments. A "shared parser" would cause your worried "slow path".

It would lose the fast-path of svg with size 16 and no classes, thought, so that is a downside and probably not worth because of that.

No, nothing is lost.

Before, for the svg, browser parse both the svgOuterHtml and svgInnerHtml

Now, we parse the svgOuterHtml, and leave the svgInnerHtml to browser.

Nothing is parsed twice.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 19, 2023

Actually, the old SvgIcon is slower, because it does create extra node or parse twice.

Old code for default arguments:

  • Vue creates a VNode (extra node)
  • Browser parses the svgOuterHtml and svgInnerHtml

Old code for non-default arguments:

  • It parses the whole SVG string to Node (svgOuterHtml and svgInnerHtml)
  • It converts the Node to string (slow)
  • Then svgOuterHtml and svgInnerHtml are parsed again by the browser, twice!

After this PR:

  • This PR first parses svgOuterHtml, it doesn't parse svgInnerHtml (fast)
  • Then it creates a VNode by svgOuterHtml parsed node, which is faster, no extra node.
  • Leave the svgInnerHtml to browser, which is faster.
  • No extra node, nothing is parsed twice.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 21, 2023
@lunny lunny added the topic/ui Change the appearance of the Gitea UI label Mar 21, 2023
@lunny lunny added this to the 1.20.0 milestone Mar 21, 2023
web_src/js/svg.js Outdated Show resolved Hide resolved
web_src/js/svg.js Outdated Show resolved Hide resolved
Co-authored-by: silverwind <me@silverwind.io>
@GiteaBot GiteaBot removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Mar 22, 2023
@GiteaBot GiteaBot added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Mar 22, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 23, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #23570 (faeaecb) into main (f521e88) will decrease coverage by 0.02%.
The diff coverage is 37.89%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main   #23570      +/-   ##
==========================================
- Coverage   47.14%   47.12%   -0.02%     
==========================================
  Files        1149     1154       +5     
  Lines      151446   152353     +907     
==========================================
+ Hits        71397    71798     +401     
- Misses      71611    72075     +464     
- Partials     8438     8480      +42     
Impacted Files Coverage Δ
cmd/dump.go 0.67% <0.00%> (ø)
cmd/web.go 0.00% <0.00%> (ø)
models/actions/run.go 1.64% <0.00%> (-0.08%) ⬇️
models/actions/runner.go 1.44% <ø> (ø)
models/packages/package.go 45.45% <0.00%> (-1.13%) ⬇️
models/user/search.go 77.50% <0.00%> (-6.29%) ⬇️
modules/actions/github.go 0.00% <0.00%> (ø)
modules/actions/workflows.go 0.00% <0.00%> (ø)
modules/context/context.go 64.54% <0.00%> (-3.53%) ⬇️
modules/doctor/storage.go 31.93% <0.00%> (ø)
... and 33 more

... and 34 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@lunny lunny merged commit 389e83f into go-gitea:main Mar 23, 2023
@lunny lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 23, 2023
@wxiaoguang wxiaoguang deleted the fix-svg-span branch March 23, 2023 03:24
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 24, 2023
* upstream/main:
  Fix incorrect `HookEventType` of pull request review comments (go-gitea#23650)
  [skip ci] Updated translations via Crowdin
  Fix codeblocks in the cheat sheet (go-gitea#23664)
  Drop migration for ForeignReference (go-gitea#23605)
  Fix new issue/pull request btn margin when it is next to sort (go-gitea#23647)
  A tool to help to backport locales, changes source strings to fix other broken translations (go-gitea#23633)
  Fix incorrect `show-modal` and `show-panel` class (go-gitea#23660)
  Restructure documentation. Now the documentation has installation, administration, usage, development, contributing the 5 main parts (go-gitea#23629)
  Check LFS/Packages settings in dump and doctor command (go-gitea#23631)
  Use a general approach to show tooltip, fix temporary tooltip bug (go-gitea#23574)
  Improve workflow event triggers (go-gitea#23613)
  Improve `<SvgIcon>` to make it output `svg` node and optimize performance (go-gitea#23570)
@silverwind silverwind mentioned this pull request Mar 25, 2023
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants