JavaScript: Closure compilation at the advanced level (WIP)

[lhecker]

Hey @haberman!

I've recently found your TODO and started adding those proper @export etc. annotations. My current work can be found here (WIP - I'll surely squash the commits in the future).

I'm actually nearly done now: All required annotations are added, the gulp file as well as the tests have been modified & fixed. The problem I now have is that you guys are using goog.asserts, which gets stripped out on the advanced closure level and there is no CLI switch to disable that (there is only setRemoveClosureAsserts(boolean) in the API).

So... What should be the way forward now? Should I simply copy/paste the required parts from goog.asserts into this project, write a modified closure-compiler CLI or simply drop all asserts in the dist release? 🙂

cc: @xfxyjwf

P.S.: The size of the gzipped dist release is about 60% smaller (28kB vs. 11kB) and permanently (!) about 30-50% (!) faster without the asserts @ advanced level. So maybe if we continue dropping all non-essential asserts we could even gain a non-trivial speed improvement (or release a "fast" version alternatively, since almost always incoming data in the browser is received by trusted entities anyways).

[myitcv]

30-50% faster? That sounds fantastic.

On that very subject, is there some way we can add Javascript benchmarks?

Having some benchmarks prior your PR being merged @lhecker would then nicely showcase the improvements when they land.

Would also help to avoid situations like pre-release protocolbuffers/protobuf#2117

Is there a general, language independent suite of benchmarks that could be used?

[lhecker]

@myitcv I'll port the C++ benchmarks to JS realy quick, but I'm not really comfortable with integrating it into this project since I'm not familar with the coding style, preferred directory layout, nor library/framework choice. I'll make it available as a gist or something though as soon as I'm done. 🙂

[lhecker]

@myitcv I've set up a benchmark project (quick & dirty though).

Decode

Here are the interesting numbers (formatted):

name	samples	ops/sec	ns/op	rme	relative
proto2 message1 old	93	169445	5902	±0.13%
proto2 message1 new	95	348173	2872	±0.62%	+105.48%
proto2 message2 old	75	135	7390579	±1.62%
proto2 message2 new	83	202	4955115	±0.34%	+49.63%
proto3 message1 old	95	119103	8396	±0.67%
proto3 message1 new	95	195243	5122	±1.12%	+63.93%

Encode

And the less interesting part (encoding doesnt really use assertions):

name	samples	ops/sec	ns/op	rme
proto2 message1 old	92	201252	4969	±0.85%
proto2 message1 new	93	204008	4902	±0.80%
proto2 message2 old	88	384	2603301	±1.10%
proto2 message2 new	90	390	2566644	±0.81%
proto3 message1 old	94	121750	8214	±0.83%
proto3 message1 new	93	121377	8239	±0.92%

[myitcv]

Thanks for the update @lhecker - we'd be very happy to give this a try when you have a branch ready.

[lhecker]

@myitcv You can find it here. 🙂

You can build it by running gulp dist in the js subfolder, which will generate a google-protobuf.js file there. It won't pass tests yet though since they rely on goog.asserts which are optimized away in my new branch. Alternatively you can use this file, which is exactly the same.

[myitcv]

@lhecker - I'm being slow; how does the google-protobuf.js that results from gulp dist correspond to the files that are distributed as part of a JS release?

[lhecker]

Ah okay I assumed you're using the npm/commonjs version (because most do nowadays, right?) and that particular file is what's distributed there. If you don't use that you can probably directly use my fork (I don't really see any difference between that JS release and the actual repository so I guess that should work).

[myitcv]

No, we're using the official Github releases

What's interesting is that the file google-protobuf.js is not included in the official release tarballs.

Which makes me wonder whether we're doing the "right thing" or not...

We reference:

<script src="goog/base.js"></script>
<script src="protobuf/map.js"></script>
<script src="protobuf/message.js"></script>
<script src="protobuf/binary/arith.js"></script>
<script src="protobuf/binary/constants.js"></script>
<script src="protobuf/binary/utils.js"></script>
<script src="protobuf/binary/decoder.js"></script>
<script src="protobuf/binary/encoder.js"></script>
<script src="protobuf/binary/reader.js"></script>
<script src="protobuf/binary/writer.js"></script>

where goog/base.js is a reference to the latest closure-library

And the protobuf/**/*.js files are those distributed as part of the official protobuf releases as I mentioned above.

Any thoughts on where we're going wrong?

Should we be referencing the google-protobuf.js file? In which case, should it be part of the official release tarballs?

[lhecker]

No I think you should continue referencing the same files but replaced with those from my fork instead. I didn't use anything except for CommonJS and ES6 modules in a long time so I'm not entirely sure about any implications regarding precompilation with the closure compiler etc. I'm not even a real frontend engineer anyways. 😄 But my gut instincts tell me that it should just work. ™

The google-protobuf.js generated by gulp dist is a CommonJS module and used as the official npm module (which is why it's not additionally included in the JS release). As such it uses module.export (like in node.js) and exports nothing to the global window object, which you're probably expecting though if you use the JS files directly. If you'd want to use google-protobuf.js you'd need to use a browser bundler like Webpack.

[lhecker]

Ah btw @myitcv: I don't know how your asset compilation chain is set up or if you even have one but to make use of possible performance improvements due to the removal of the goog.asserts calls you'd need to use either the precompiled CommonJS module or compile the assets you listed above using the Closure Compiler like this. Otherwise you'll always continue using the debug-by-default code.

You can also try this experimental file. It's the same as google-protobuf.js, but I replaced the only occurrence of module.exports with window.jspb={} at the end of the file. It could possibly work as a replacement to all the files you include above. If not you could make a similar change (by replacing module.exports with something else).

[myitcv]

@lhecker thanks. It's been a while since we looked at our build pipeline... indeed when we first put it together there we no up-to-date npm packages for either the closure library or protobuf (hence my not having a clue about google-protobuf.js) so great to see that's moved on. I'll take a closer look over the next couple of days in order to try and exercise your optimisations.

[myitcv]

@lhecker after a very rudimentary test, we're also seeing ~40% speed up which is great, specifically around deserializeBinaryFromReader

[haberman]

Wow, this is great. Sorry for the delay, There is so much activity on GitHub that I miss stuff sometimes.

I think dropping asserts for the dist release should be fine. We want them to trigger in unit tests, but I think the intention is that release builds will not need them.

Please send a PR with your work -- this sounds great.

[lhecker]

@haberman Sorry I forgot about this again. 😕
But I took some time today to rebase my work onto the current master, which you can find here.

There where quite some merge conflicts going on, but by carefully applying some nuclear regular expressions (as simple as /\*\*\n((?!@export)(.|\n))+? \*\/\n[.\w]+?[^_] =) it was easy to fix. 😂

Again: The problem is that throughout the code you guys currently use closure asserts as a way to add preconditions for functions, but unfortunately asserts get stripped away in optimized builds.

So my question in this issue remains: What do we do about the use of asserts in the code? One example is this which makes the testCopyInto_notSameType test fail. This is the only test that fails due to this, but there are surely more places in the code where asserts are used like that, but don't have any unit tests yet.

The other test that fails right now is testUnknownExtension, but I'm not yet entirely sure why it fails. This test creates a new jspb.BinaryWriter(); which is not coming from the optimized google-protobuf.js, because if you dump the instance to the console it's structure looks like this:

writer { blocks_: [],
  totalLength_: 0,
  encoder_: { buffer_: [] },
  bookmarks_: [] }

while a BinaryWriter from the compiled version looks like this:

 V { f: [],
   b: 0,
   a: Xa { a: [] },
   g: [] }

I don't really understand all the code in this library, which is why it'd be great if someone can help me that test in particular.

Do you want me to still open a PR even though it's incomplete? Either way we have to decide what to do about the assertions (e.g. drop them including tests that rely on that behaviour entirely or replace them with if (...) throw new Error()) and how to solve the testUnknownExtension test. 🙂

[haberman]

Again: The problem is that throughout the code you guys currently use closure asserts as a way to add preconditions for functions, but unfortunately asserts get stripped away in optimized builds.

I think this is entirely intended. The asserts are intended to catch errors in how the functions are called. But we only want to perform these checks in debug builds (ie. in unit tests). In optimized builds we don't want to pay for these checks.

How many of our unit tests depend on asserts getting triggered? Perhaps we could wrap these checks somehow so that they are only triggered for non-opt builds?

The other question is how we could allow our users to use the non-opt build for their own unit tests. Ideally the npm package could include both debug and release builds, and users could opt for which one they want. Does that seem doable?

[AndrewGuenther]

ping @lhecker would love to see these changes land.

Looking at your WIP branch it wasn't immediately clear, do these changes only apply to the google-protobuf module, or would these options be available through protoc as well?

[lhecker]

@AndrewGuenther I'm currently very busy and don't have enough time to work on this. I'm not even sure when this will change in the future... Especially considering that I recently switched to compiling the output of protoc with the js source of protobuf by using the closure compiler directly (producing a very compact output). This achieves an even slightly better result compared to my fork and makes this issue a secondary one for me as it's not a pressing one anymore. I hope you can understand that. 😅
Maybe even someone from Google pick up my work and finish the remaining issues with the unit tests?

The WIP branch (js-closure-advanced) only applies to the google-protobuf module on npm as well as the output you get when compiling from the source directly (like me above). The JS code generated by protoc is already fully compatible with closure's advanced optimization level and no further options are needed there. Or did I possibly misunderstand your question?

[lhecker]

@haberman As I said in my previous comment right above I'm currently extremely busy and don't know when I'm able to continue my work. I hope you can understand that as well. 😅

The asserts might be a must have for the npm module though as that one won't have any closure type annotations etc. It'd might indeed be a good idea to ship a debug and a release build there due to that. I don't think there is an easy solution for switching between both though. A user of the npm package would have to code an own solution for that using a build script etc.

Also regarding the tests relying on asserts: I'm only aware of testCopyInto_notSameType and testUnknownExtension relying on that. I went into a bit into detail in my other comment above.

[AndrewGuenther]

@lhecker that makes sense, I actually ended up going a similar route as well, however, when I directly compile the protoc output, none of the exported symbols seem to exist. Did you run into any issues with this?

Currently, I have to compile the entirety of each consuming project together in order to make it work, but ideally I would like to only compile the protoc output.

[lhecker]

@AndrewGuenther Did you make sure to use the --export_local_property_definitions and --generate_exports flags when calling the closure compiler? Otherwise exported properties won't actually be exported in the advanced level.

The relevant parts in my build script look like this:

if [ ! -r "$deps_path" ]; then
	echo $'\n# Calcdeps'
	./node_modules/google-closure-library/closure/bin/calcdeps.py \
		-p ./node_modules/google-closure-library/closure \
		-p "$project_dir" \
		-i "$js_entry_path" \
		> "$deps_path"
fi

readarray -t deps < "$deps_path"

java -jar "$compiler_path" \
	--warning_level=VERBOSE \
	--jscomp_error='*' \
	--jscomp_off=analyzerChecks \
	--jscomp_off=deprecated \
	--jscomp_off=extraRequire \
	--jscomp_off=inferredConstCheck \
	--jscomp_off=lintChecks \
	--jscomp_off=unnecessaryCasts \
	--compilation_level=ADVANCED \
	--process_common_js_modules \
	--assume_function_wrapper \
	--language_in=ECMASCRIPT5 \
	--language_out=ECMASCRIPT5 \
	--generate_exports \
	--export_local_property_definitions \
	--rewrite_polyfills=false \
	--create_source_map="$out_map_path" \
	--js_output_file="$out_js_path" \
	"${deps[@]}"

The extra call to calcdeps.py is needed in my case because I'm also using clutz, which needs a list of input files to generate a .d.ts file for TypeScript. If that's not needed you can add this instead of having "${deps[@]}".

[jjbubudi]

Merry Christmas everyone!

Looks like this went quiet for quite a while but it will be very useful to have it fixed. I've raised PR protocolbuffers/protobuf#5509 for this. Working on the tests.

[lhecker]

Oh dear... I'm such an idiot... I seem to have deleted my protobuf fork at some point since opening this issue and my work from back then has been lost. 😰
Thanks for picking up on this though, @jjbubudi! protocolbuffers/protobuf#5509 looks really nice. 🙂

[jjbubudi]

@lhecker Your commit is still here (referenced in this issue) :)
lhecker/protobuf@9e94cae
But the branch itself is gone so I have trouble finding out which commit is modifying the tests.

[jjbubudi]

After further modifications the only failing test is testCopyInto_notSameType which depends on goog.asserts being available even after compilation. To fix it properly might require some not-so-trivial code or build script changes.

[dhlolo]

Is there any progress? Seem jj's PR is not merged, and proto.js is still too large for mobile cases.