Skip to content

Conversation

@ascholerChemeketa
Copy link
Contributor

@ascholerChemeketa ascholerChemeketa commented Dec 29, 2024

The match logic for 'webwork-rep-to-static' was causing significant slowdowns. This reimplements to favor quick matching and then filtering on the matched elements.

Produces clean diffs on webwork sample chapter and reduces build time from ~10s to under 8s.

On APEX Calculus, this and get_publisher_varaible reduce the build time of APEX from ~20m to ~2.5m.

@rbeezer
Copy link
Collaborator

rbeezer commented Dec 30, 2024

Yes, there are a few complicated expressions I'd love to get a handle on. This is one of them. Not sure I've ever totally understood it, and hoped that it might even become (already is?) obsolete as we get better stuff back from servers. Anyway, I'm not sure I totally understand its purpose, so I'd like to ask @Alex-Jordan to have a hard look as part of a review.

Looks good to me, in principle, but I've not done a full review yet.

@Alex-Jordan
Copy link
Contributor

Commenting about the changes to the old line 2571:

  • I can follow the logic of the changes and it seems good. I don't understand the internals of why assessing the $prune variable is so much faster than using predicates, but I believe it.
  • Is it right that the old not(normalize-space(text())) becomes just not(normalize-space())? I can no longer recall if there is an important difference there. I guess because we have already passed the tests for only having one child, and that child being a fillin, it is equivalent?
  • not(parent::li) or (parent::li and preceding-sibling::*) is faithful to what was there before, but would it be equivalent (yet faster) to do not(parent::li) or preceding-sibling::*? Or maybe even faster to swap the order and do preceding-sibling::* or not(parent::li)?

@Alex-Jordan
Copy link
Contributor

There are also the changes here to var[@form=/...]. I can't review right now, but I will. This is where there is probably a conflict with #2336, but it will be easy to resolve. This PR can go in first and I'll patch #2336 as needed.

@ascholerChemeketa
Copy link
Contributor Author

(not(parent::li) or preceding-sibling::*) is a good reduction of the last clause. It appears that the existing order fo the subclasses (parent then sibling) is slightly faster, though there isn't much real difference.

not(normalize-space()) should be the same as long as that one fillin element has no content. Putting text() back in won't harm anything though if that is not a safe assumption.

@rbeezer
Copy link
Collaborator

rbeezer commented Jan 1, 2025

Wanted to get this in now. Questions:

is a good reduction of the last clause.

Can't see that this has happened. Will wait to do further work until I hear about this.

Testing outputs of sample chapter (HTML, LaTeX). This must be happening in the assembly phase when constructing the static version (since it is a preview for HTML use). The "?" is new, and I don't think we want it. There's another exercise that shows this also.

-<li id="extracted-webwork-75-1-1-1-1-1"><div class="para" id="extracted-webwork-75-1-1-1-1-1-1">Red</div></li>
-<li id="extracted-webwork-75-1-1-1-1-2"><div class="para" id="extracted-webwork-75-1-1-1-1-2-1">Blue</div></li>
-<li id="extracted-webwork-75-1-1-1-1-3"><div class="para" id="extracted-webwork-75-1-1-1-1-3-1">Green</div></li>
+<li id="extracted-webwork-75-1-1-1-1-1"><div class="para" id="extracted-webwork-75-1-1-1-1-1-1">?</div></li>
+<li id="extracted-webwork-75-1-1-1-1-2"><div class="para" id="extracted-webwork-75-1-1-1-1-2-1">Red</div></li>
+<li id="extracted-webwork-75-1-1-1-1-3"><div class="para" id="extracted-webwork-75-1-1-1-1-3-1">Blue</div></li>
+<li id="extracted-webwork-75-1-1-1-1-4"><div class="para" id="extracted-webwork-75-1-1-1-1-4-1">Green</div></li>
+\item{}?%
 \item{}Red%
 \item{}Blue%
 \item{}Green%

@Alex-Jordan
Copy link
Contributor

It looks like the added \item{}?% is because before this commit, the template is looking for a var[@form='popup'] with an li that has '?' as its text content. But the li has been overlooked in the changes. The new version looks for a var with @form='popup' and the var's text content would be '?'. Which is not matched here because the text content of the var includes all the other options.

Side note: the older mechanism in PG for creating a popup list only presents the choices the author wrote. Authors did not like seeing the HTML select element be pre-populated with the first of their answer options, so authors started making '?' be their first option. Fine for HTML rendering, but not for static PTX. Why have a bullet choice that is a '?'?

A newer mechanism for doing this lets PG authors write only the actual choices, and separately uses a placeholder text attribute for the '?'. When that is used, we (PTX) do not worry about the unwanted '?' showing up. If all/most PG problems could evolve to use this newer method, we could remove this awkward question mark filter from here. But I think that is a future project.

@rbeezer
Copy link
Collaborator

rbeezer commented Jan 3, 2025

Would it be possible to get this one sorted? I'd love to get this speed-up in soon for everybody building WW problems.

@ascholerChemeketa ascholerChemeketa force-pushed the webwork-rep-to-static-speedup branch from b55eb20 to 4db8405 Compare January 3, 2025 16:55
@ascholerChemeketa
Copy link
Contributor Author

Did a force push with the updated clause in the p matching and drops changes to the var[@form] stuff. The speedup from them was very small in comparison to the p.

@rbeezer
Copy link
Collaborator

rbeezer commented Jan 3, 2025

Thanks! Should merge soon.

@rbeezer
Copy link
Collaborator

rbeezer commented Jan 3, 2025

Excellent! Thanks for another speed-up. I'll let @sean-fitzpatrick know!

@rbeezer rbeezer closed this Jan 3, 2025
@sean-fitzpatrick
Copy link
Contributor

I'm looking forward to trying it out!

@sean-fitzpatrick
Copy link
Contributor

Down to 3 and a half minutes!
That might be less than it takes for one pass of xelatex on the PDF build!

@ascholerChemeketa ascholerChemeketa deleted the webwork-rep-to-static-speedup branch January 15, 2025 16:35
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.

4 participants