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

Pre-process Build #1296

Merged
merged 42 commits into from
Dec 18, 2013
Merged

Pre-process Build #1296

merged 42 commits into from
Dec 18, 2013

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Nov 15, 2013

Adds RequireJS pragmas to conditionally include code that throws DeveloperErrors like:

//>>includeStart('tag', condition);
if (/*condition*/) {
    new DeveloperError('something went wrong.');
}
//>>includeEnd('tag');

tag can be any string describing the block, but the tags in each includeStart/includeEnd must be the same. condition can be any javascript that is evaluated at build time. pragmas.debug will be true for debug builds or false for release builds.

Adds two new build targets, combineRelease and minifyRelease, which are the same as combine and minify except they defined pragmas.debug to be false.

Adds pragma statements like above to every function in Context, Cartesian*, Matrix* and Quaternion that will be removed when pragmas.debug is false.

The performance was improved by ~2-4% for the most frequently called functions. The minified size decreased by ~0.02 MB.

@@ -46,6 +50,31 @@
<antcall target="generateDocumentation" />
<antcall target="buildApps" />
</target>

<target name="combineRelease" depends="build" description="Combines all source files into a single stand-alone script with debugging code removed.">
<antcall target="combineJavaScript">
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a tab in here. Sorry. The Eclipse formatter should be safe to run on the whole file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, this file uses tabs exclusively, I forgot. In any case, the formatter should be fine.

@shunter
Copy link
Contributor

shunter commented Nov 15, 2013

You should add the <arg value="pragmas.debug=${pragmas.debug}" /> option to the r.js commands that create the workers too.

@bagnell
Copy link
Contributor Author

bagnell commented Nov 15, 2013

@shunter I wasn't sure what else should have it. Everything that uses r.js?

@shunter
Copy link
Contributor

shunter commented Nov 15, 2013

Well, the places where we use r.js to minify CSS doesn't need it. Otherwise, yeah. I think for buildCesiumViewer we can always do debug=false.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 15, 2013

Add the new build targets to the Contributor's Guide.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 15, 2013

We need links in index.html to be able to run with these?

Also are we required to use the combined version? I thought we were going to setup an AMD version just like code coverage?

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 15, 2013

I thought we had to make changes to the tests so they still pass in release?

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 15, 2013

I stepped through Scene.render and added pragmas for most the developer errors, include a few that where still called per-command.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 15, 2013

The performance was improved by ~2-4% for the most frequently called functions.

I don't know what this means. We microbenchmarked individual functions? We care about performance in practice for scenes with lots of draw commands.

Compare both FPS and the profiles for:

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 15, 2013

@shunter can do the merge once this is ready.

@bagnell
Copy link
Contributor Author

bagnell commented Dec 3, 2013

@shunter This is ready for another review.

@bagnell
Copy link
Contributor Author

bagnell commented Dec 3, 2013

This also addresses #1294.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 11, 2013

Any update here?

@shunter
Copy link
Contributor

shunter commented Dec 18, 2013

As discussed offline, I fixed a few problems and added info to CHANGES. Merging.

shunter added a commit that referenced this pull request Dec 18, 2013
@shunter shunter merged commit 2bd4f29 into master Dec 18, 2013
@shunter shunter deleted the preprocess branch December 18, 2013 22:30
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.

3 participants