fix: Remove triple parsing of diagrams

[sidharthv96]

📑 Summary

Many diagrams were parsed 2-3 times for each render. Removed that.

📏 Design Decisions

Moved the call to init() after clear().

📋 Tasks

Make sure you

• 📖 have read the contribution guidelines
• 💻 have added necessary unit/e2e tests.
• 📓 have added documentation. Make sure MERMAID_RELEASE_VERSION is used for all new features.
• 🔖 targeted develop branch

[codecov]

Codecov Report

Merging #4697 (da8112a) into develop (bcb0817) will increase coverage by 26.79%.
The diff coverage is 93.33%.

Additional details and impacted files

@@             Coverage Diff              @@
##           develop    #4697       +/-   ##
============================================
+ Coverage    46.03%   72.82%   +26.79%     
============================================
  Files           53      146       +93     
  Lines         6736    14658     +7922     
  Branches        18      562      +544     
============================================
+ Hits          3101    10675     +7574     
- Misses        3635     3851      +216     
- Partials         0      132      +132     

Flag	Coverage Δ
e2e	77.98% <85.71%> (?)
unit	46.08% <61.53%> (+0.05%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
packages/mermaid/src/diagrams/er/erRenderer.js	93.47% <ø> (ø)
.../mermaid/src/diagrams/sequence/sequenceRenderer.ts	62.46% <ø> (ø)
packages/mermaid/src/mermaidAPI.ts	59.79% <0.00%> (+16.17%)	⬆️
packages/mermaid/src/Diagram.ts	66.66% <100.00%> (+26.66%)	⬆️
packages/mermaid/src/diagram-api/diagramAPI.ts	69.62% <100.00%> (+13.92%)	⬆️
.../mermaid/src/diagrams/flowchart/flowRenderer-v2.js	72.64% <100.00%> (ø)
...ges/mermaid/src/diagrams/state/stateRenderer-v2.js	92.94% <100.00%> (ø)

... and 133 files with indirect coverage changes

[sidharthv96]

Time taken to render diagrams in demos/flowchart.html

Browser	Before	After	Decrease
Chromium	2289.7900001049043	1945.0699999332428	15%
Firefox	1157.3	971.8	16%
Webkit	2626.2	2525.5	3.8%

Before Chromium render: 2157.5 ms render: 2163.2000002861023 ms render: 2151.5 ms render: 2160.699999809265 ms render: 2709.1000003814697 ms render: 2367.5 ms render: 2637.7000002861023 ms render: 2195.1000003814697 ms render: 2227.4000000953674 ms render: 2128.199999809265 ms Average: 2289.7900001049043 Firefox render: 1245 ms render: 1133 ms render: 1161 ms render: 1150 ms render: 1167 ms render: 1137 ms render: 1151 ms render: 1154 ms render: 1125 ms render: 1150 ms Average: 1157.3 Webkit render: 2758 ms render: 2637 ms render: 2653 ms render: 2614 ms render: 2602 ms render: 2549 ms render: 2588 ms render: 2610 ms render: 2595.0000000000005 ms render: 2656 ms Average: 2626.2

After Chromium render: 1963.9000000953674 ms render: 1891.0999999046326 ms render: 1998 ms render: 1934.7999997138977 ms render: 1982.5 ms render: 1944.7999997138977 ms render: 1950.5 ms render: 1915.5 ms render: 1911.5 ms render: 1958.0999999046326 ms Average: 1945.0699999332428 Firefox render: 1037 ms render: 954 ms render: 1026 ms render: 943 ms render: 954 ms render: 939 ms render: 973 ms render: 986 ms render: 964 ms render: 942 ms Average: 971.8 Webkit render: 2535 ms render: 2583 ms render: 2557 ms render: 2536 ms render: 2540 ms render: 2554 ms render: 2512 ms render: 2479 ms render: 2486 ms render: 2473 ms Average: 2525.5

---

Interesting to see Firefox is twice as fast as Chrome for rendering mermaid!

[sidharthv96]

Related

• Remove double parsing #4587

[Yokozuna59]

Awesome!

I saw some diagrams have init function in it's definition, and it's clearing the DB there, is it related to this PR? i.e., flowDiagram-v2.

[sidharthv96]

Awesome!

I saw some diagrams have init function in it's definition, and it's clearing the DB there, is it related to this PR? i.e., flowDiagram-v2.

Yeah, I noticed that, but didn't want to change too much in this PR. That can be removed and tested in an upcoming flowDB cleanup PR (that I'm working on).

[nirname]

Awesome. What I previously did was only initial cleanup of extra rendering, and you opened a can of warms for sure. Nice excavations.

[aloisklink]

Wooo, the performance impact sounds pretty nice!

The only potential issue I see is whether or not this is a BREAKING CHANGE.

In some of the tests, you've removed lines that used to have await mermaidAPI.parse(str1);. Is this something that might break how Mermaid's public API works?

[Yokozuna59]

The only potential issue I see is whether or not this is a BREAKING CHANGE.

In some of the tests, you've removed lines that used to have await mermaidAPI.parse(str1);. Is this something that might break how Mermaid's public API works?

I guess it's not a breaking change. When initializing new diagram using new Diagram(), it runs the parse function async and update the DB, which is exactly what the mermaidAPI do when calling mermaidAPI.parse.

[Yokozuna59]

After looking at these changes, doesn't that mean that there's no use of text parameter in draw function expect logging what diagram is being rendered with the actual diagram text?

I guess we could add that log call inside the render function with id, something like this:

log.debug(`rendering ${id} diagram\n${text}`);

So maybe now we could remove unnecessary parameters in v11, see #4450 (comment).

[sidharthv96]

The only potential issue I see is whether or not this is a BREAKING CHANGE.

In some of the tests, you've removed lines that used to have await mermaidAPI.parse(str1);. Is this something that might break how Mermaid's public API works?

I don't think it'll have any external changes visible to the mermaidAPI.parse users.

---

So maybe now we could remove unnecessary parameters in v11, see #4450 (comment).

Yes, we could! The renderer should not need to know anything regarding the original text. It's only concern should be rendering whatever is in the DB.

[aloisklink]

I don't think it'll have any external changes visible to the mermaidAPI.parse users.

In that case, I'll officially approve this PR :)

---

So maybe now we could remove unnecessary parameters in v11, see #4450 (comment).

Yes, we could! The renderer should not need to know anything regarding the original text. It's only concern should be rendering whatever is in the DB.

Should we make a GitHub issue (or maybe a GitHub Discussion is better) for all of the breaking changes we want in a v11 release?

There are some breaking changes I want to make to Mermaid.JS types :)

[aloisklink]

Side-note, what label should we give this PR for the Release notes? Type: Bug / Error or Type: Other?

Maybe we should make a new label called Type: Performance, since most JavaScript projects use a different label for performance stuff (e.g. see @commitlint/config-conventional, which uses perf: xxxx for commits that affect performance).

[Yokozuna59]

Should we make a GitHub issue (or maybe a GitHub Discussion is better) for all of the breaking changes we want in a v11 release?

I think we should. We are currently just suggesting stuff in random issues and PRs without collecting and filtering them. It would also help us create a changelog for v11.

[sidharthv96]

Done: https://github.com/orgs/mermaid-js/discussions/4710
Also updated the next branch, which we could use for the breaking changes.

[knsv]

Good Job @sidharthv96 !