-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
rewrite svg renderer #836
rewrite svg renderer #836
Conversation
…ox >= 39. - To resolve the case where the chart uses SVG renderer and has shadow filter.
fix: spelling error
fix(type): improve gradient types
fix(svg): svg mouse event doesn't work normally in Firefox when using shadow.
fix: export Displayable for type annotation compatibility
fix prototype pollution in merge, clone, extend utilities
Prepare to release 5.2.1
Release 5.2.1
fix: always close path on circle
Merge release to master
fix: optimize arraydiff perf in first render and clear
Added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks really promising to me! I can't wait to see it in future releases!
const res = /^([0-9]*?)px$/.exec(font); | ||
const fontSize = +(res && res[1]) || DEFAULT_FONT_SIZE; | ||
let width = 0; | ||
if (font.indexOf('mono') >= 0) { // is monospace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may be fonts with 'mono'
in name but are not monospace fonts, like Monoton. And it may be necessary to convert the name into lowercase before checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another problem is, would font
contains multiple font families like '12px Lato, Source Han Sans SC'
? This may be especially helpful when developers want to set font families for multiple charsets, like in this case, Lato for Latin characters and Source Han Sans SC for Chinese.
If this is the case, then even if one of the font family is monotype doesn't necessarily mean that characters in other font families share the same width.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure, is defaultMeasureText
responsible for returning width that is as accurate as possible or just a rough result? If the answer is the later, maybe my previous considerations are not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only a very rough fallback in node environments that don't have any other methods to measure text.
In fact, we won't suggest developers using fonts that are not system-provided in the node server. If so, they need to use registerCanvas
method and use node-canvas
to get an accurate size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure but will it be better if I do accurate check monospace
instead of fuzzy font name? Just like serif
and sans-serif
.
// Generated from following code | ||
// | ||
// ctx.font = '12px sans-serif'; | ||
// const asciiRange = [32, 126]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it help if we also caches the width of '国'
to represent Chinese character width? In the current implementation, fontSize
is used as width in this case, which may not be necessarily accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We assume Chinese characters all have the same width as fontSize here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks really promising to me! I can't wait to see it in future releases!
TLDR;
This PR rewrites our SVG renderer to provides better server-side rendering and brings a significant performance improvement.
What's the Problem Now
In our current implementation, there are two major issues:
we render to SVG element from zrender graphic element directly. To avoid too much DOM manipulation, we add a lot of diff and cache strategies. But it seems it's still not enough and there are still lots of unnecessary DOM manipulation. But we rather keep the code simpler instead of doing more comparing and caching to improve the performance a bit. The reason of this choice is because the logic of converting from zrender element to SVG element is complex. Especially when it comes to shadow, clipPath, pattern, etc. These special effects are very different between SVG and Canvas. Doing cache can easily make the code very complex and buggy.
We choose the Myers' Diff Algorithm to do elements diff. As described in fix: optimize arraydiff perf in first render and clear #832 . This algorithm will degenerate very fast when the elements changes lot. So like in first render, or elements are removed and added freqeuently. The performance becomes terrible.
How we Fixed it in This PR
At first this branch is not aiming to improve the performance of SVG renderer. It's for providing a new
svg-ssr
renderer. Which can do string-based SVG render and get rid of JSDom or node-canvas in the NodeJS environment. In this renderer we render all the elements to a VNode representation. Then convert these VNodes to SVG string. Each render is full and there is no cache or diff algorithm is used. Because of this, the code is simple and robust. Then I think we got VNode now. Why don't we use a virtual dom library and patch it to the container every frame? So I tried Snabbdom and the performance surprise me. The FPS is about 1 / 3 higher in the case of updating all elements in every frame. When it comes to the cases that elements are added or removed frequently. The FPS can be even 10x higher.Here are two simple examples:
Before:
https://zrender-svg-ssr.glitch.me/svg.html
After:
https://zrender-svg-ssr.glitch.me/svg2.html
Why This Method is Fast
New implementation will render all elements into vnodes that use the following structure.
This approach is fast because there is no DOM manipulation. Then patching algorithm from Snabbdom will render this vnodes into DOMs. Because mapping from vnode to DOM is very simple. The patching can diff all the attributes and only apply the changed attributes. The extra cost that new algorithm brings is it will create vnode for all elements in every frame. It will bring much pressure to the GC. But it turns out this extra cost is still worth it. The total render time is still much less.
What's New in This new Renderer
Because we will do full render to vndoes every frame now. The logic can be much simpler and robust. Turns out the new algorithm fixes some render bugs that we didn't notice. For example, shadows may be lost, text may not be removed, etc.
And we can now do server-side rendering with embedded CSS animation without need of JSDOM or node-canvas.
An example of rendered SVG
We can apply more optimizations to reduce the size of generated SVG. For example, put same attributes into a single class.
Other Changes?
Some compatible code for ancient browsers are removed:
Default
measureText
method can calculate text width withoutnode-canvas
. It's only accurate on default font. If developer is using other font that are very different on the metrics, it will be better to still usenode-canvas
TypeScript is upgraded to 4.4
How to test this Branch in Apache ECharts
Checkout svg-ssr branch in echarts