Protobuf usage in sourcemaps?

[co...@colinalworth.com]

I'm trying to make use of the sourcemap tooling in closure-compiler, and something caught my eye - the sourcemap json format is produced and consumed through the use of GSON, but there is some use of protobuf in handling some of the internals. The mapping.proto file defines the OriginalMapping message and other details, but as far as I can tell, those proto objects are never serialized or deserialized, but only used as a sort of immutable/builder structure. Would a patch be accepted to replace the generated ~1500 lines of java output from proto with a corresponding AutoValue to serve the same purpose?

I know this wouldn't remove other .proto usage in the compiler, but as far as I can tell, most/all of those other protos are actually read/written, rather than used only as 'structs for java with extra steps'? Or perhaps there is more context when sharing code internally with Google that I'm missing?

Thanks,

Colin

[Bradford Smith]

Hi Colin,

I'm not certain, but you may well be right that we don't currently serialize / deserialize OriginalMapping objects.

We do, however, have logic that serializes and deserializes the entire compiler state so that compilation can be broken up into 3 stages for large projects that take too long for a single action and to save repeated work when multiple output localizations are needed.

That serialization / deserialization is at the moment a combination of a fully protocol buffer representation of the AST (Abstract Syntax Tree) and standard Java serialization for the rest of the compiler's state. This state includes sourcemap information, though I'm not 100% certain the particular `OriginalMapping` message is used.

This is a rather ugly solution that was chosen only because it was the most expedient. There is, though, a general trend to move more and more of the serialized state into protocol buffers, because that is the preferred data serialization format at Google.

So, I think the answer to your question is: No, we wouldn't accept a PR that convered `OriginalMapping` to an AutoValue.

That would be going against the direction we generally want to go.

Now, perhaps I'm wrong, and `OriginalMapping` really is only used as some kind of temporary data object, so having it be a proto is wasted effort.

If so, I'm afraid the burden would be on the PR author to demonstrate how we can be certain of this, exactly, and even more importantly, show that making the change to AutoValue would give some benefit that is worth the effort and risk of making the change.

Thanks for asking.

Best regards,

Bradford

[co...@colinalworth.com]

As it happens, I just finished the patch - it was easier than I thought, so I'm happy to shelve it. I'll see if I can make the case in a coherent way that gets along with the other serialized state things I've run into (as far as I can tell, it doesn't, or at least not yet...), and go from there. The benefit is mostly only that it seems like using a wrench to fasten a button - the OriginalMapping structure is so light (no repeateds, only the one message), it basically seems to serve as a tuple for a language that doesn't yet have them...

Thanks for the feedback - I'm not hopeful that it'll be accepted, but I'll give it a shot since it was straightforward.

[John Lenz]

It was originally used by some other Google infrastructure for decoding stack traces.  The author of that code was fond protobufs.  I'm sure that if we got some benefit we could translate the proto to POJO somewhere along the way outside the compiler's codebase.

--

---
You received this message because you are subscribed to the Google Groups "Closure Compiler Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to closure-compiler-d...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/closure-compiler-discuss/b19fee95-c016-4089-8bd7-32703baf4d6en%40googlegroups.com.

[co...@colinalworth.com]

Here's the pull request: https://github.com/google/closure-compiler/pull/4152

I thought I had seen earlier that the values (but not the message itself) were being used in some way by CompilerState, but I can't find them now and believe I was mistaken. The closest they seem to come is Compiler.getSourceMapping, which is used to let consumers of Compiler read the sourcemaps it knows about and obtain information for reporting messages to the user. 

Our purpose is in adding a (non-shaded) dependency on closure-compiler to a server to let it parse sourcemaps, without also getting the current protobuf version (and potentially needing to update it or resolve conflicts). I recognize that this isn't the purpose of the compiler and its internals, but given the above and the (very very slightly) simplification that the patch brings, I thought it could be worth a try.

Thank you for your time in considering and discussing this,

Colin

[Bradford Smith]

I expect to examine the PR this week.