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

Minify GLSL shaders #13706

Open
wants to merge 1 commit into
base: branch-3.5
Choose a base branch
from
Open

Conversation

ianthomas23
Copy link
Member

Fixes #13335.

This minifies the WebGL shaders, mostly by removing comments and unnecessary whitespace. Example of shader before minifying:

precision mediump float;

attribute vec2 a_position;
attribute vec4 a_bounds;

uniform float u_pixel_ratio;
uniform vec2 u_canvas_size;

varying vec2 v_tex_coords;

void main()
{
  v_tex_coords = vec2(a_position.x < 0.0 ? 0.0 : 1.0, a_position.y < 0.0 ? 0.0 : 1.0);

  float x = a_position.x < 0.0 ? a_bounds[0] : a_bounds[2];
  float y = a_position.y < 0.0 ? a_bounds[1] : a_bounds[3];
  vec2 xy = vec2(x, y);

  vec2 pos = xy + 0.5;  // Bokeh's offset.
  pos /= u_canvas_size / u_pixel_ratio;  // in 0..1
  gl_Position = vec4(2.0*pos.x - 1.0, 1.0 - 2.0*pos.y, 0.0, 1.0);
}

and after:

precision mediump float;attribute vec2 a_position;attribute vec4 a_bounds;uniform float u_pixel_ratio;uniform vec2 u_canvas_size;varying vec2 v_tex_coords;void main(){v_tex_coords=vec2(a_position.x<0.?0.:1.,a_position.y<0.?0.:1.);float x=a_position.x<0.?a_bounds[0]:a_bounds[2];float y=a_position.y<0.?a_bounds[1]:a_bounds[3];vec2 xy=vec2(x,y);vec2 pos=xy+.5;pos/=u_canvas_size/u_pixel_ratio;gl_Position=vec4(2.*pos.x-1.,1.-2.*pos.y,0.,1.);}

Bundle sizes on branch-3.4:

    - bokeh-gl.js          :  551.5 KB
    - bokeh-gl.min.js      :  204.7 KB

and after this PR:

    - bokeh-gl.js          :  551.5 KB
    - bokeh-gl.min.js      :  186.9 KB

so it saves 17.8 KB of the gl-min bundles, about 8.7%.

I looked at existing NPM packages for this and didn't find any I liked. Mostly they are unmaintained and tightly coupled to webpack. So I wrote my own as a sequence of regex replacements. These should by understandable by anyone (including my future self) with some regex knowledge. The multiple passes of each shader, one per regex, are probably not very quick but GLSL is such a small fraction of the BokehJS codebase that there isn't any measurable increase in build time on my dev machine.

In future we could extend the minifying to replace local function and variable names with smaller strings, but that would require storing state so would need a different implementation.

Testing is a problem. I think that the visual baseline tests use the unminified bundles so this will not be tested by CI. Could you confirm this @mattpap?

Copy link

codecov bot commented Feb 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0021c18) 92.58% compared to head (76d2eff) 92.58%.

Additional details and impacted files
@@             Coverage Diff             @@
##           branch-3.4   #13706   +/-   ##
===========================================
  Coverage       92.58%   92.58%           
===========================================
  Files             325      325           
  Lines           20729    20729           
===========================================
  Hits            19192    19192           
  Misses           1537     1537           

@mattpap
Copy link
Contributor

mattpap commented Feb 20, 2024

Are unminified shaders of any use, e.g. for debugging? If not, and I presume this is the case, then I would always minify them. So, I would move the minification function to bokehjs/make/tasks/scripts.ts and apply it on 44. This way no assumptions will be needed about the number of scripts per module and we will keep the minified version under test.

@mattpap mattpap added this to the 3.4 milestone Feb 20, 2024
@mattpap
Copy link
Contributor

mattpap commented Feb 20, 2024

Also for reference, we always minify generated CSS from *.less files, so doing the same for GLSL would make things consistent, again assuming that there is no utility for unminified version.

@ianthomas23
Copy link
Member Author

Are unminified shaders of any use, e.g. for debugging?

Yes they are. If you write an invalid shader that fails to compile on the GPU you get an error report in the browser console with the line number of the error. You would still see that when using a minified shader but it would be harder to work out which line number it corresponds to in the original unminified shader.

@mattpap
Copy link
Contributor

mattpap commented Feb 26, 2024

Yes they are. If you write an invalid shader that fails to compile on the GPU you get an error report in the browser console with the line number of the error. You would still see that when using a minified shader but it would be harder to work out which line number it corresponds to in the original unminified shader.

I see. Ideally this would be handled via a source map, but that's a lot of work and it's probably not worth it in this case. Let's proceed with this PR.

@mattpap mattpap modified the milestones: 3.4, 3.4.1 Feb 28, 2024
@mattpap mattpap changed the base branch from branch-3.4 to branch-3.5 March 14, 2024 16:50
@mattpap mattpap modified the milestones: 3.4.1, 3.5 Mar 26, 2024
@bryevdv
Copy link
Member

bryevdv commented May 15, 2024

@mattpap what do you want to do with this PR? It looks like it could be merged with a trivial MyPy fix.

@ianthomas23
Copy link
Member Author

It looks like it could be merged with a trivial MyPy fix.

Not yet. The minified shaders are not used in CI so we could be generating and shipping garbage without knowing it.

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.

Minify WebGL shaders
3 participants