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

Kb 48270 #990

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Kb 48270 #990

wants to merge 2 commits into from

Conversation

xjiang-at-wiris
Copy link
Contributor

Description

Optimized the performance of the viewer by implementing the following:

  1. Added an Interaction Observer to avoid rendering all functions on the page simultaneously. Instead, it renders only the currently visible functions and some that are just outside the viewport.
  2. Added a Mutation Observer to render newly added functions individually instead of re-rendering the entire page. This also allows for the quick conversion of LaTeX to MathML.
  3. Enabled concurrent rendering of multiple functions within a set upper limit, ensuring all visible functions on the page are rendered in parallel.

Steps to reproduce

  1. Launch viewer on local yarn && nx build viewer && nx start html-viewer or in kitchen
  2. Try to put a lot of formulas, you can use this
  3. Ensure all mencioned before are working
  4. Try to modify new added parameters on table simultaneousmml simultaneouslatex vieweroffset

#48270

feature: load formula asynchronous

feat(viewer): add observer in order optimize performance

feat: viewer improvement
Copy link
Contributor Author

@xjiang-at-wiris xjiang-at-wiris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some explanation

};

window.viewer.properties.config = config;

document.getElementsByTagName("main")[0].innerHTML = document.getElementById("content").value;
window.com.wiris.js.JsPluginViewer.parseElement(document.getElementsByTagName("main")[0]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been removed because we no longer need "Render under request."

@@ -48,6 +48,41 @@ function decodeSafeMathML(root: HTMLElement) {
}
}

export async function renderAMathML(properties: Properties, mathElement: MathMLElement): Promise<void> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to delete the renderMathML function, but it seems to have a lot of implications and requires many document modifications. However, it appears worth discussing its feasibility.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like so too. I would even consider this part of the development of the task, since the renderMathML is used for the retro-compatibility methods, which could benefit from the new performance improvements.

It would require a part of investigation and maybe adapt the current methods so that renderMathML can be properly replaced.

@@ -112,6 +112,9 @@ The following table contains a specification of each of the properties.
| lang | The language for the alt text. | Frontend | | en |
| dpi | Resolution in dots per inch of the generated image. This feature scales the formula with a factor of dpi/96. | Frontend | Positive integer | 96 |
| zoom | The scale of the generated image. | Frontend | Positive floating point number | 1 |
| vieweroffset | Number of pixels to render in advance. | Frontend | Positive floating point number | 200 |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm missing maybe some information on what would that parameter be of use, because if I'm using the viewer for the first time, with that information I would not understand.

Maybe add something like "We render only the formulas on screen. This parameter defines the ..."

@@ -112,6 +112,9 @@ The following table contains a specification of each of the properties.
| lang | The language for the alt text. | Frontend | | en |
| dpi | Resolution in dots per inch of the generated image. This feature scales the formula with a factor of dpi/96. | Frontend | Positive integer | 96 |
| zoom | The scale of the generated image. | Frontend | Positive floating point number | 1 |
| vieweroffset | Number of pixels to render in advance. | Frontend | Positive floating point number | 200 |
| simultaneousmml | Mmax number of simultaneous MathML rendering petition can make at the same time. | Frontend | Positive floating point number | 50 |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| simultaneousmml | Mmax number of simultaneous MathML rendering petition can make at the same time. | Frontend | Positive floating point number | 50 |
| simultaneousmml | Max number of possible simultaneous MathML rendering petitions at the same time. | Frontend | Positive floating point number | 50 |

@@ -112,6 +112,9 @@ The following table contains a specification of each of the properties.
| lang | The language for the alt text. | Frontend | | en |
| dpi | Resolution in dots per inch of the generated image. This feature scales the formula with a factor of dpi/96. | Frontend | Positive integer | 96 |
| zoom | The scale of the generated image. | Frontend | Positive floating point number | 1 |
| vieweroffset | Number of pixels to render in advance. | Frontend | Positive floating point number | 200 |
| simultaneousmml | Mmax number of simultaneous MathML rendering petition can make at the same time. | Frontend | Positive floating point number | 50 |
| simultaneouslatex | max number of simultaneous LaTeX rendering petition can make at the same time. | Frontend | Positive floating point number | 50 |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| simultaneouslatex | max number of simultaneous LaTeX rendering petition can make at the same time. | Frontend | Positive floating point number | 50 |
| simultaneouslatex | Max number of possible simultaneous LaTeX rendering petitions at the same time. | Frontend | Positive floating point number | 50 |

// Interaction Observer
const mathobserver = new IntersectionObserver(async (entries, observer) => {

let latexPromises = [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you are using the simultaneousmml but I fail to find the place where the simultaneouslatex is being used. It seems then that the parameter is not providing any value to the viewer at this point. Is this intentional, or am I failing to see something?

Copy link
Contributor

@psala-at-wiris psala-at-wiris Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here!
In Latex.ts, line 21:

 const maxLatexPetition = properties.simultaneousmml;

Maybe maxLatexPetition should be equal to properties.simultaneouslatex?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake! It should be const maxLatexPetition = properties.simultaneouslatex;
FIxed, thank you both!

@@ -48,6 +48,41 @@ function decodeSafeMathML(root: HTMLElement) {
}
}

export async function renderAMathML(properties: Properties, mathElement: MathMLElement): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like so too. I would even consider this part of the development of the task, since the renderMathML is used for the retro-compatibility methods, which could benefit from the new performance improvements.

It would require a part of investigation and maybe adapt the current methods so that renderMathML can be properly replaced.

@carla-at-wiris
Copy link
Contributor

I'm getting the following console errors on the viewer demo:
image
image


const allElements = document.querySelectorAll('*');
window.addEventListener("load", function () {
let counter = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this counter used for something?
Looks like a development tool XD

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted! Thank you!

for (const entry of entries) {

if (entry.isIntersecting && entry.target instanceof MathMLElement && entry.target.matches('math')) {
const promise = renderAMathML(properties, entry.target);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I am missing something, but:
Why the renderAMathMl method does not implement the petition guard and renderLatex does?
Could be possible to implement the guard logic in the same way for both methods? For me is kind of confusing that one does the guard mechanism and the other doesn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we talked in private, renderAMathMl and renderLatex have some different behavior, that's why they don't share the same guard mechanism

@usantos-at-wiris
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants