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

shader compilation speedup #9497

Merged
merged 2 commits into from
May 1, 2020
Merged

shader compilation speedup #9497

merged 2 commits into from
May 1, 2020

Conversation

sgolbabaei
Copy link
Contributor

@sgolbabaei sgolbabaei commented Apr 2, 2020

After some investigation, we realized that some of the calls to getActiveAttrib, getActiveUniform, getProgramParameter and getAttribLocation since these are the information we can get from our styling pipeline and program configuration.

The changes that is being introduced with this change:

Removed the layout Attributes from program configuration since it is no longer being used in program class. With the new changes, we are now getting the run time attributes and uniforms information from binders. The only dependency to gl we have is for getUniformLocation.

These are some of the benchmark results which compares this local branch to master:
Layer Line:
LayerLine

Layer Circle:
LayerCircle

Layer Fill:
LayerFill

Paint:
Paint

Also, this is a screen shot of the performance with recent changes. You can see that the main time consuming operation is regarding getUniformLocation:

image

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • post benchmark scores
  • manually test the debug page

@sgolbabaei sgolbabaei requested a review from kkaefer April 7, 2020 18:08
@karimnaaji karimnaaji added this to the release-e milestone Apr 7, 2020
Copy link
Contributor

@kkaefer kkaefer left a comment

Choose a reason for hiding this comment

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

This looks good overall!

The approach of using regular expressions for parsing attribute/uniform information is somewhat brittle. We should consider using an approach where we parse the shaders at build time and stores the metadata as constant data in the JS bundle. We could e.g. parse the shaders in our build script with headless-gl, then use our current code to extract the names. Alternatively, a native shader compiler could work as well for extracting metadata.

What does the setFeatureState-error branch in your graphs represent? Is that this PR?

src/render/program.js Outdated Show resolved Hide resolved
src/data/program_configuration.js Outdated Show resolved Hide resolved
src/data/program_configuration.js Outdated Show resolved Hide resolved
src/render/program.js Outdated Show resolved Hide resolved
src/shaders/shaders.js Outdated Show resolved Hide resolved
src/shaders/shaders.js Outdated Show resolved Hide resolved
Copy link
Contributor

@arindam1993 arindam1993 left a comment

Choose a reason for hiding this comment

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

Looks good @sgolbabaei , we just need to address the problem with using Set

src/render/program.js Outdated Show resolved Hide resolved
@sgolbabaei
Copy link
Contributor Author

@arindam1993 hmm.. It seems like some specific functionalities of Sets are not supported in IE11:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set

Since I am just using the constructor and that is supported, we should be OK. Right?

@kkaefer
Copy link
Contributor

kkaefer commented Apr 28, 2020

What's the problem with Set in IE11?

getting Attribute info

some clean-up

Code clean-up

Addressing PR comments

move Regex into the parameter list

Remove using Set since some the functionalities of Set is not supported in IE11.
@sgolbabaei
Copy link
Contributor Author

@arindam1993 @kkaefer Do you think it is possible with these changes to link an unused attribute to position 0?

@arindam1993
Copy link
Contributor

Capturing slack convo:

Should be fine since staticUniforms, are first in the list.

@sgolbabaei
Copy link
Contributor Author

@arindam1993 @kkaefer

I think we should be good with this changes. We don't need to bring back the layoutAttributes, since they are basically the equivalent of the static attributes in this case.

For instance, in case of fill_extrusion_attributes (src/data/bucket/fill_extrusion_attributes.js) is basically manually created instances of the static attributes. Since in the code base we do staticAttrInfo.concat(dynamicAttrInfo) we should be good. The only case which could get tricky is when static attribute is empty which is basically the case when layout attribute is empty.. (same functionality as the previous code)

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