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

Add js compiler protodef implementation, that is 10x faster #102

Merged
merged 29 commits into from
May 20, 2020

Conversation

Karang
Copy link
Contributor

@Karang Karang commented May 11, 2020

My attempt at #86

The compiler can generate reading and writing code. It follows the protodef standard and can be extended with custom native types. It can compile a code that is able to read the whole nbt spec.

I also included an experimental function that can optimize the generated code using the closure compiler. It is not always faster and, when it is, not by much.

Running examples/compiled.js:

Running 1000000 tests
write / read compiled: 195.35 ms (5119.09k packet/s)
serializer / parser: 18095.48 ms (55.26k packet/s)
write / read compiled (+closure): 123.37 ms (8105.85k packet/s)

Running examples/compiled_nbt.js:

Running 10000 tests
read compiled: 282.89 ms (35.35k packet/s)
parser: 2237.11 ms (4.47k packet/s)
read compiled (+closure): 288.36 ms (34.68k packet/s)

@rom1504
Copy link
Member

rom1504 commented May 11, 2020 via email

@Karang
Copy link
Contributor Author

Karang commented May 11, 2020

That's cool. Did you try yet to read minecraft protocol.json with this or are there missing features that prevent it ?

As far as node-protodef is concerned, nothing is missing. But to implement the minecraft protocol, the specific primitives needs to be implemented : https://github.com/PrismarineJS/node-minecraft-protocol/blob/master/src/datatypes/minecraft.js (nbt is already done in my example). But following the example of https://github.com/ProtoDef-io/node-protodef/pull/102/files#diff-9e714e61d25d5e6f12b4fcd39795423fR64 it shouldn't be hard to do.

I included the nbt example here (and copy pasted some code from prismarine-nbt) for testing purpose, but in reality, the non- node-protodef code should be in its respective repository.

So, to support minecraft protocol, we need to first update prismarine-nbt then node-minecraft-protocol. (I can already implement the missing bits and include them here for testing, then we can move it later)

@Karang Karang force-pushed the karang_compiler branch from f539566 to 7897c47 Compare May 11, 2020 20:51
@rom1504
Copy link
Member

rom1504 commented May 11, 2020

ok I get it, would be good to add a .md file similar to https://github.com/ProtoDef-io/node-protodef/blob/master/doc/newDatatypes.md that would explain how to add these types

}

function optimize (code, cb) {
const closureCompiler = new ClosureCompiler({
language_in: 'ES6',
compilation_level: 'SIMPLE'
Copy link
Contributor

Choose a reason for hiding this comment

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

Umm, did 'ADVANCED' mode breaks code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is too aggressive with variable renaming, even when specifying the correct externs. There might be a way to make it work but a lot of configuration would be required.

@Karang Karang force-pushed the karang_compiler branch from 1a8e728 to 20d36ae Compare May 15, 2020 19:55
Copy link
Member

@rom1504 rom1504 left a comment

Choose a reason for hiding this comment

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

overall it looks good

it would be nice to run the tests using it

doc/compiler.md Show resolved Hide resolved
doc/compiler.md Show resolved Hide resolved

## New datatypes

Like the ProtoDef interpreter, the ProtoDef compiler can be extended with custom datatypes. To register a custom type, use the `addTypes(types)` method of the ProtoDef compiler. The `types` parameter is an object with the following structure:
Copy link
Member

Choose a reason for hiding this comment

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

what do you think of grouping the types (each with its read, write, sizeof) instead of doing groups of read/write/sizeof containing each all the types ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I can do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this was done to simplify the implementation: internally there is 3 separate compilers for each functionality, it's just easier to give them each their own types. We can go around that with a conversion step but that's a bit of work I prefer to do later.

doc/compiler.md Show resolved Hide resolved
doc/compiler.md Show resolved Hide resolved
doc/compiler.md Show resolved Hide resolved
@rom1504
Copy link
Member

rom1504 commented May 15, 2020

Overall API looks ok, so I think it makes sense to start integrating directly in prismarine-nbt and node-minecraft-protocol (you can git depend on node-protodef for now in PRs)

@rom1504
Copy link
Member

rom1504 commented May 16, 2020

would be nice to adapt the unit tests (https://github.com/ProtoDef-io/node-protodef/tree/master/test/dataTypes) and benchmarks (https://github.com/ProtoDef-io/node-protodef/tree/master/benchmark) to run on both the interpreter and the compiler (separately)
it'll make it possible to catch any future regression + to do more precise benchmarking whenever needed

if (!isNaN(fn)) {
this.primitiveTypes[type] = fn
this.native[type] = (value) => { return fn }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we inline native functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case fn is a number (!isNaN(fn)) It cannot be inline because it needs to have the same function interface, but anyway the JIT compiler will optimise it for us.

@Karang
Copy link
Contributor Author

Karang commented May 19, 2020

would be nice to adapt the unit tests (https://github.com/ProtoDef-io/node-protodef/tree/master/test/dataTypes) and benchmarks (https://github.com/ProtoDef-io/node-protodef/tree/master/benchmark) to run on both the interpreter and the compiler (separately)
it'll make it possible to catch any future regression + to do more precise benchmarking whenever needed

done

@rom1504 rom1504 changed the title WIP: investigate js code generation from protodef json Add js compiler protodef implementation, that is 10x faster May 20, 2020
@rom1504
Copy link
Member

rom1504 commented May 20, 2020

LGTM, thanks for this huge improvement !

@rom1504 rom1504 merged commit 70641a8 into ProtoDef-io:master May 20, 2020
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