search

catapult-project / catapult

Deprecated Catapult GitHub. Please instead use http://crbug.com "Speed>Benchmarks" component for bugs and https://chromium.googlesource.com/catapult for downloading and editing source code..
https://chromium.googlesource.com/catapult
BSD 3-Clause "New" or "Revised" License
1.93k stars 563 forks source link

[wpr-go] Add Brotli support #3742

Open xunjieli opened 7 years ago

xunjieli commented 7 years ago

https://github.com/kothar/brotli-go/tree/master is not pure golang code. This caused issues for cross compiling, so we removed it in https://codereview.chromium.org/2989713002/.

Filing a bug to add back Brotli support.

xunjieli commented 7 years ago

master bug: #3629

xunjieli commented 7 years ago

/cc @eustas

tombergan commented 7 years ago

What are the issues in cross-compiling? The "official" Go wrapper around the C code is here: https://github.com/google/brotli/tree/master/go/cbrotli

@dsnet has a pure-Go implementation of Brotli, but I'm not sure what state it's in. https://github.com/dsnet/compress/tree/master/brotli

dsnet commented 7 years ago

dsnet/compress only has a Brotli decoder. I would love to implement an encoder at some point, but it's a complex format and I don't have spare cycles to take on that project.

In terms of the decoder implementation, I'm pretty confident in it's correctness. It's gone through many months of fuzzing comparing it's implementation to the C version (and actually discovered bugs in the C version google/brotli#502) and was a project that helped verify the specification for the RFC finalization.

However, there is much room for performance improvements. It's about half the speed last I checked.

xunjieli commented 7 years ago

/cc @nedn I don't know the details, but we weren't able to get kothar/brotli compiled correctly for Windows with cross compiling. I was able to get it compiled and run on my windows machine, but the binary doesn't work for trybots. We didn't spend too much time on it. Folks who are using Golang in Chrome infra told us having pure Golang dependencies will make updates much easier.

@eustas might be able to see if we can integrate cbrotli in wprgo. WebPageReplay needs both the decoder and encoder. If cbrotli doesn't work, we can wait until dsnet/compress is ready.

tombergan commented 7 years ago

I actually have code to integrate cbrotli with wpr-go. The code change is trivial, the hard part is integrating with the build system. There are two ways to do that:

  1. Require that the brotli C libs are installed in the local machine. Then cbrotli should "just work" automatically via this cgo file: https://github.com/google/brotli/blob/master/go/cbrotli/cgo.go

  2. Build wpr-go with Bazel, and integrate with cbrotli's Bazel BUILD rules: https://github.com/google/brotli/blob/master/go/cbrotli/BUILD

I'm not sure which is better ... that will depend on how wpr-go is being installed on the test and benchmarking machines.

nedn commented 7 years ago

The main problem with cgo we are having is that make it's harder to do cross complication. Building go binary on every platform is quite tedious :-/

eustas commented 7 years ago

What scenarios are tested? If no real compression is required, then brotli encoder would be just a dozen lines - tiny header and one byte footer will turn bytes to a proper "brotli" stream...

xunjieli commented 7 years ago

We need a fully functional Brotli encoder and decoder. The scenario is that when Chrome advertises "br" in Accept-Encoding, and facebook gives us brotli-encoded response. WprGo needs to be able to decompress that content, inject some custom javascript (e.g. deterministic.js), and re-compress that content in Brotli encoded format.