This is a refreshed version of bug 830881's attachment 734627 [details] [diff] [review], updated to r97 of LZ4. LZ4 has gained a few new decompression functions. I didn't hook those up because it isn't yet clear to me if we actually have use for them. Using LZ4 for JS source compression will probably inform that.

Are you sure that MOZ_ASSERT is what you want in MOZ_ASSERT(inputSizeChecked.isValid()); Don't you want to check for validity also in release builds, and return false on invalid?

Comment on attachment 769413 [details] [diff] [review] Add LZ4 compression to mfbt Review of attachment 769413 [details] [diff] [review]: ----------------------------------------------------------------- I'm moderately assuming this is unchanged from previous patches I've reviewed. ::: mfbt/Compression.cpp @@ +3,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "mozilla/Compression.h" > +#include "mozilla/CheckedInt.h" > +using namespace mozilla::Compression; #include "mozilla/CheckedInt.h" #include "mozilla/Compression.h" and a blank line after those before the |using|. @@ +58,5 @@ > + return true; > + } else { > + *outputSize = 0; > + return false; > + } Just make this *outputSize = ret; return ret >= 0; ::: mfbt/Compression.h @@ +8,5 @@ > +#ifndef mozilla_Compression_h_ > +#define mozilla_Compression_h_ > + > +#include "mozilla/Types.h" > +#include "mozilla/Assertions.h" Alphabetize these. @@ +11,5 @@ > +#include "mozilla/Types.h" > +#include "mozilla/Assertions.h" > + > +namespace mozilla { > +namespace Compression { Should be |namespace compression|, for lowercase namespace naming. @@ +27,5 @@ > + > +class LZ4 > +{ > + > +public: class contents want to be indented two more spaces. And get rid of the blank lines before/after public:. @@ +33,5 @@ > + /** > + * Compresses 'inputSize' bytes from 'source' into 'dest'. > + * Destination buffer must be already allocated, > + * and must be sized to handle worst cases situations (input data not compressible) > + * Worst case size evaluation is provided by function LZ4_compressBound() That should be LZ4::maxCompressedSize(inputSize), right? @@ +102,5 @@ > + @return maximum output size in a "worst case" scenario > + */ > + static MFBT_API size_t maxCompressedSize(size_t inputSize) > + { > + size_t max = ((inputSize) + ((inputSize)/255) + 16); inputSize + inputSize / 255 + 16;

Regarding the input size, it's an API requirement that a valid size be passed in. I think it would be reasonable for us to submit a patch upstream that changed all the size arguments to be size_t, to avoid this issue, and made lz4 itself handle these error cases. But given the massive sizes we're talking about, it probably doesn't matter enough in practice to force error-handling of this to be an impediment to landing.

I'm starting to think mfbt is the wrong place for this, and that this should go into mozglue/something. The end result is the same, as mfbt ends up in mozglue, although one main difference is that at the moment js/src doesn't handle mozglue. But my point is that mfbt is starting to grow into something it's not supposed to be, but that mozglue is.

I tentatively agree: mfbt isn't exactly a perfect fit for this. OTOH, getting shell compilation to integrate mozglue is work we might not want to prevent this from landing. Waldo, what do you think?

I view mfbt as a place to dump code and algorithms for common use throughout Gecko and SpiderMonkey. lz4 compression would seem to fit that bill, in my mind. So I don't immediately see why this would belong in mozglue and not in mfbt. I don't know enough about mozglue and its ultimate purpose to know why this might belong there instead. I'd agree about not letting mozglue integration hold up work here, tho (perfect is enemy of good and all that), based on what I understand now. It's possible if I learned more I might change my mind, but I don't know what I don't know to know how likely that is. :-)

Would it be acceptable to publish LZ4 as prefixed non-namespaced |extern "C"| in mfbt? We need C bindings for bug 854169, so we can either have them here or build them on top of the C++ mfbt bindings.

Mmh, that is a good question. One that I can't really answer, so I'll defer to Waldo.

I'd prefer you write your own minimal C bindings, to be honest. C bindings mean we lose the benefits of things like smart pointers, ranges passed by struct rather than by (separated) pointer and length, and so on. We shouldn't give up those benefits just because an isolated user can't tolerate them, as a general rule.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11) > I'd prefer you write your own minimal C bindings, to be honest. C bindings > mean we lose the benefits of things like smart pointers, ranges passed by > struct rather than by (separated) pointer and length, and so on. We > shouldn't give up those benefits just because an isolated user can't > tolerate them, as a general rule. I'm not implying that we should not have C++ bindings. I'm just looking for the best way to also have C bindings alongside the C++ bindings. We can either have the low-level C bindings or rebuild them – probably more awkwardly – on top of the C++ bindings. Also, vladimir hasn't reviewed the patch in two months. Are we still waiting for him?

Vlad, are you ok with us landing this, or do you want to do a review, too? It's basically an updated version of your patch from bug 830881.

Comment on attachment 769413 [details] [diff] [review] Add LZ4 compression to mfbt Land away! Gah, sorry, I didn't realize that it was waiting for my r+.

Patch seems bitfresh. Try: https://tbpl.mozilla.org/?tree=Try&rev=8afe997e6c95

Doesn't build under Windows.

Just needed __debugbreak() -> ::__debugbreak() for C++.

Can we please get this landed?

Pushed to try here: https://tbpl.mozilla.org/?tree=Try&rev=f94cc21a8ea4 If that turns up green, I'll land. (After getting an r+ from Waldo on IRC again.)

Let's try this again with less breakage in preceding pushes. remote: https://tbpl.mozilla.org/?tree=Try&rev=80089f0b1bef

Comment on attachment 817212 [details] [diff] [review] Assertions.h fixup for windows Review of attachment 817212 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/Assertions.h @@ +180,4 @@ > # ifdef __cplusplus > # define MOZ_REALLY_CRASH() \ > do { \ > + ::__debugbreak(); \ Uh, people are writing classes/namespaces with __debugbreak members? Blech.

Well, somehow: e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/mfbt/Compression.cpp(50) : error C2668: '__debugbreak' : ambiguous call to overloaded function predefined C++ types (compiler internal)(163): could be 'void __debugbreak(void)' c:\tools\msvs10\vc\include\intrin.h(159): or 'void `anonymous-namespace'::__debugbreak(void)' while trying to match the argument list '(void)' I bet this is happening because we #include lz4.c inside an anonymous namespace, which is pulling in intrin.h inside there. ::__debugbreak() is valid regardless though, so may as well use it.

Finally! remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/df958bc8af6d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1de6b6604c11

https://hg.mozilla.org/mozilla-central/rev/df958bc8af6d https://hg.mozilla.org/mozilla-central/rev/1de6b6604c11