You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some browsers limit the size of modules that can be compiled synchronously because that blocks the main thread.
One implication of this is that compiling an autodiff graph becomes an async operation, so in particular, compileStyle (and thus also compileTrio) becomes async.
Another implication is that loading/initializing the optimizer itself is an async operation. We considered three possible architectures to accommodate this:
Don't initialize the optimizer Wasm module at load time, and instead, make each of the functions exposed by the Wasm module wrapper have an implicit precondition that the module must have already been loaded; this has the benefit that we don't need top-level await and we don't need to make all those functions async, but the drawback that we now need to remember to properly initialize the module before every possible usage or we'll have race conditions.
Change the interface of the Wasm module wrapper so that rather than exposing things directly, it exposes just one async function that initializes the Wasm module and returns an object with methods to access all the underlying Wasm functions; this has the advantage of not requiring top-level await and not giving things implicit preconditions, but the viral disadvantage that now every consumer of the Wasm module needs to either be async or take in this object as a parameter.
Use top-level await, and update our downstream packages to use ESM instead of CJS; this has the advantage of no race conditions, no implicit preconditions, and no viral changes to downstream function signatures, but the disadvantage that docs-site needs to support top-level await.
This PR takes approach (1), but we would like to switch to (3) in the future if possible; see the further discussion below.
Checklist
I have commented my code, particularly in hard-to-understand areas
My changes generate no new ESLint warnings
I have reviewed any generated changes to the diagrams/ folder
Open questions
To reduce the time to compile the WebAssembly to native code, a followup should identify sets of objectives/constraints that are topologically similar (usually those sharing a source location in the Style program) and compile those to a single WebAssembly function, instead of duplicating their logic across many functions.
If a generated gradient function crashes while computing polynomial roots, it does not undo the change it made to the stack pointer. I don't know of a good way to handle this. But on the other hand, I don't know whether it's possible for such a crash to occur.
While developing this PR, I added ${FORCE_COLOR+--color=true} to @penrose/optimizer's esbuild script, and ${FORCE_COLOR+--color=always} to its cargo scripts, to get color output in Nx. I've removed those because they're not portable to Windows, but it would be nice to come up with a portable way to get this back in the future; see this issue: Some scripts aren't colored when run by Nx #1176
Originally I expected that this PR would allow us to handle a thousand points in our hundred-points-around-star example (which would previously yield a stack overflow), but instead, after this PR it simply crashes the whole page. See Stack overflow with too many substance objects #1070
In the medium term, it would be nice to remove all our instances of await ready; by using a top-level await in @penrose/optimizer. As far as I can tell, this would require us to get rid of Webpack first; see docs: Migrate site from Docusaurus to VitePress #1172
I removed the autodiff debug node, because I don't think we ever use it, and it would have taken nontrivial effort to port to WebAssembly. I can add it back if we want, but didn't want to waste time if we don't care.
This PR uses a prerelease version of wasm-bindgen that includes the --keep-lld-exports flag and does not emit BigInt literals. Once version 0.2.84 is released, we should just switch to that, but for now, the CONTRIBUTING.md instructions just say to install from GitHub at a specific commit, and our CI/CD scripts pull a Linux binary which I compiled and put in a GitHub Gist.
Although we mostly compile Wasm asynchronously for reasons described above, there are two exceptions: in tests, and for convex partitioning. The former is fine because we only run tests in Node and not in the browser, but the latter may become an issue if we end up using convex partitioning on polygons with too many points or whose point locations are computed via sufficiently complicated expressions.
As you can verify by running strings packages/optimizer/target/wasm32-unknown-unknown/release/penrose_optimizer.wasm, the built WebAssembly binary contains absolute paths (starting with /Users/samueles/.cargo/registry/src/github.com-1ecc6299db9ec823/ in my case) to local files on whatever machine built the binary. See this Rust issue: Enable --remap-path-prefix for absolute paths by default rust-lang/rust#40552
Merging #1092 (2caa419) into main (831a598) will increase coverage by 0.83%.
The diff coverage is 96.04%.
❗ Current head 2caa419 differs from pull request most recent head 275b29c. Consider uploading reports for the commit 275b29c to get more accurate results
M 3d-projection-fake-3d-linear-algebra.svg
M circle-example-euclidean.svg
M collinear-euclidean.svg
M congruent-triangles-euclidean.svg
M continuousmap-continuousmap.svg
M hypergraph-hypergraph.svg
M incenter-triangle-euclidean.svg
M lagrange-bases-lagrange-bases.svg
M midsegment-triangles-euclidean.svg
M non-convex-non-convex.svg
M one-water-molecule-atoms-and-bonds.svg
M parallel-lines-euclidean.svg
M persistent-homology-persistent-homology.svg
M points-around-line-shape-distance.svg
M points-around-polyline-shape-distance.svg
M points-around-star-shape-distance.svg
M siggraph-teaser-euclidean-teaser.svg
M small-graph-disjoint-rect-line-horiz.svg
M small-graph-disjoint-rects-small-canvas.svg
M small-graph-disjoint-rects.svg
M tree-tree.svg
M tree-venn-3d.svg
M tree-venn.svg
M two-vectors-perp-vectors-dashed.svg
M vector-wedge-exterior-algebra.svg
M wet-floor-atoms-and-bonds.svg
M word-cloud-example-word-cloud.svg
M wos-laplace-estimator-walk-on-spheres.svg
M wos-nested-estimator-walk-on-spheres.svg
M wos-offcenter-estimator-walk-on-spheres.svg
M wos-poisson-estimator-walk-on-spheres.svg
While developing this PR, I added ${FORCE_COLOR+--color=true} to @penrose/optimizer's esbuild script, and ${FORCE_COLOR+--color=always} to its cargo scripts, to get color output in Nx. I've removed those because they're not portable to Windows, but it would be nice to come up with a portable way to get this back in the future.
@samestep can you write an issue about this? Perhaps @liangyiliang can implement a workaround for Windows since he is our main Windows user.
Originally I expected that this PR would allow us to handle a thousand points in our hundred-points-around-star example (which would previously yield a stack overflow), but instead, after this PR it simply crashes the whole page. See #1070
@samestep can you update 1070 so it explains the new issue.
While developing this PR, I added ${FORCE_COLOR+--color=true} to @penrose/optimizer's esbuild script, and ${FORCE_COLOR+--color=always} to its cargo scripts, to get color output in Nx. I've removed those because they're not portable to Windows, but it would be nice to come up with a portable way to get this back in the future.
@samestep can you write an issue about this? Perhaps @liangyiliang can implement a workaround for Windows since he is our main Windows user.
Originally I expected that this PR would allow us to handle a thousand points in our hundred-points-around-star example (which would previously yield a stack overflow), but instead, after this PR it simply crashes the whole page. See #1070
@samestep can you update 1070 so it explains the new issue.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR improves optimizer performance by an order of magnitude, building on my optimizer performance experiment from earlier this year.
Implementation strategy and design decisions
This PR adds the following build tools to our setup:
wasm-bindgento generate JavaScript/TypeScript scaffolding around Wasm compiled from RustQuoting from Stack Overflow:
One implication of this is that compiling an autodiff graph becomes an
asyncoperation, so in particular,compileStyle(and thus alsocompileTrio) becomesasync.Another implication is that loading/initializing the optimizer itself is an
asyncoperation. We considered three possible architectures to accommodate this:awaitand we don't need to make all those functionsasync, but the drawback that we now need to remember to properly initialize the module before every possible usage or we'll have race conditions.asyncfunction that initializes the Wasm module and returns an object with methods to access all the underlying Wasm functions; this has the advantage of not requiring top-levelawaitand not giving things implicit preconditions, but the viral disadvantage that now every consumer of the Wasm module needs to either beasyncor take in this object as a parameter.await, and update our downstream packages to use ESM instead of CJS; this has the advantage of no race conditions, no implicit preconditions, and no viral changes to downstream function signatures, but the disadvantage thatdocs-siteneeds to support top-levelawait.This PR takes approach (1), but we would like to switch to (3) in the future if possible; see the further discussion below.
Checklist
diagrams/folderOpen questions
WebAssembly.Module"can be efficiently shared with Workers, and instantiated multiple times", a followup should allow the optimizer to be run in a web worker; see this issue: Providing a positive user experience for slow Penrose trios #1095${FORCE_COLOR+--color=true}to@penrose/optimizer'sesbuildscript, and${FORCE_COLOR+--color=always}to itscargoscripts, to get color output in Nx. I've removed those because they're not portable to Windows, but it would be nice to come up with a portable way to get this back in the future; see this issue: Some scripts aren't colored when run by Nx #1176hundred-points-around-starexample (which would previously yield a stack overflow), but instead, after this PR it simply crashes the whole page. See Stack overflow with too many substance objects #1070await ready;by using a top-levelawaitin@penrose/optimizer. As far as I can tell, this would require us to get rid of Webpack first; see docs: Migrate site from Docusaurus to VitePress #1172debugnode, because I don't think we ever use it, and it would have taken nontrivial effort to port to WebAssembly. I can add it back if we want, but didn't want to waste time if we don't care.wasm-bindgenthat includes the--keep-lld-exportsflag and does not emitBigIntliterals. Once version 0.2.84 is released, we should just switch to that, but for now, theCONTRIBUTING.mdinstructions just say to install from GitHub at a specific commit, and our CI/CD scripts pull a Linux binary which I compiled and put in a GitHub Gist.strings packages/optimizer/target/wasm32-unknown-unknown/release/penrose_optimizer.wasm, the built WebAssembly binary contains absolute paths (starting with/Users/samueles/.cargo/registry/src/github.com-1ecc6299db9ec823/in my case) to local files on whatever machine built the binary. See this Rust issue: Enable --remap-path-prefix for absolute paths by default rust-lang/rust#40552