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

Plotly - device-independent and stable codegen + codegen cache. #637

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

yujin-wu
Copy link

@yujin-wu yujin-wu commented Jan 19, 2022

Issue

#638

  • It is currently not possible to have regl run in a CSP strict environment due to violation of unsafe-eval. This change allows developers to pre-generate shaders for their apps and make them CSP compliant.
  • In applications using regl, especially ones using many shaders, the same blocks of code can be generated multiple times but end time a new almost-duplicate (or fully duplicate) function is generated.

Changes

  • Made generated code stable - the same code is generated for the same shader regardless of device, construction order, gl attributes or limits.
    -- added "stable link" capability to link runtime-dependent values.
    -- rolled-up the vertex array loop.
  • global cache of generated code, indexed by sha256 of code.

This makes it possible for developers to create CSP compliant apps using regl, roughly by:

  1. generate stable code of all shaders used by the app, by running the app and invoking all the shaders.
  2. Extract the generated code from the __regl_codegen_cache, and save into application source.
  3. On application initialization, load the generated code into the __regl_codegen_cache. Regl will avoid codegen for those functions.

Impact on performance

  • There is small performance hit on codegen time, mainly due to a sha256 pass.
  • There is minimal impact (or even improvement) on execution performance for a single shader - stable variable lookups take marginally longer, but the rolled-up vertex array loop runs faster in many cases.
  • There is performance improvement for applications using multiple regl shaders at a time - a lot of codegen now produce identical code, and those identical code return reused functions.

@yujin-wu yujin-wu changed the title Plotly - device-independent and stable codegen, codegen cache. Plotly - device-independent and stable codegen + codegen cache. Jan 19, 2022
@rreusser
Copy link
Member

Thanks for the PR! It would be great to fix the CSP issues! I don't currently have time for an in-depth review, but a couple things which jump out at me are:

  • Since regl doesn't require a browser, I don't think window.__regl_codegen_cache will work since window may not exist. I think any caching would need to be scoped to the specific regl instance.
  • It looks like crypto isn't currently handled correctly by rollup. I see a warning like this:
Treating 'crypto' as external dependency
No name was provided for external module 'crypto' in options.globals – guessing 'crypto'

with the result that dist/regl.js tries to call a function require to locate it, which doesn't exist:

(function (global, factory) {
    typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory(require('crypto')) :
    typeof define === 'function' && define.amd ? define(['crypto'], factory) :
    (global.createREGL = factory(global.crypto));
}(this, (function (crypto) { 'use strict';

A way this has been handled in the past was to write a browserify transform which could be run to transform a dynamic call into a static call. See, for example cwise-transform.js which replaces the entry point to replace dynamic calls with code generation to static calls with the generated code. Unfortunately, the ecosystem is fragmented and browserify transforms can cause lots of headaches with various bundlers.

If anything though, I think it'd be maybe preferable to replace interaction with a global window.__regl_codegen_cache with proper methods like regl.getCachedCode() and regl.preloadCachedCode(), after which you could load it however you like.

@rreusser
Copy link
Member

Ah, I think static-module was the module which replaces module usage with inline expressions. Plotly includes it via brfs, though I'm not sure if it's actively used already. https://github.com/plotly/plotly.js/blob/master/package-lock.json#L1156

@yujin-wu
Copy link
Author

yujin-wu commented Jan 26, 2022

Thanks for taking a look!

I have added the cache to window before because I thought maybe applications have libraries that instantiate regl itself which wasn't accessible to the application. But alas I haven't seen an example of that, and it is nicer to keep it contained so as you've said I've changed that now.

I didn't even realize the crypto issue until you mentioned it, because the resulting code did happen to work wherever web cryptography existed :P. But yeah it ought to be fixed - I am still replacing it with a dependency-free safe hash function (hopefully lighter than sha256). I am still considering whether using an attackable hash/digest function to index cached code is a security concern here, if so I may end up having to include a crypto library just for this purpose.

…standalone sha256 implementation, added codegen initialization to constructor
package.json Outdated
@@ -43,7 +43,7 @@
"getusermedia": "^1.3.7",
"git-commits": "^1.2.0",
"git-parse-commit": "^1.0.0",
"gl": "4.0.1",
"gl": "4.9.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering why this change is necessary?
If not, please revert it.

Copy link
Author

Choose a reason for hiding this comment

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

Yes this was in the process of debugging the build, it is not necessary change. Reverting.

@@ -1,19 +1,14307 @@
{
"name": "regl",
"version": "2.1.0",
"lockfileVersion": 1,
"lockfileVersion": 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not touch the version of package-lock in this PR.

Copy link
Author

Choose a reason for hiding this comment

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

This has been reverted now too. I just undid the last 2 commits.

@yujin-wu yujin-wu force-pushed the plotly/cache-codegen branch 2 times, most recently from 1cd7107 to 29c9b09 Compare February 7, 2022 07:09
@CallumNZ
Copy link

CallumNZ commented Mar 6, 2022

So great that this is being worked on, scattergl working with a strong CSP will be awesome!

@yujin-wu yujin-wu marked this pull request as draft March 9, 2022 12:32
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.

None yet

6 participants