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

Switch to ProtoDefc #45

Closed
wants to merge 1 commit into from
Closed

Switch to ProtoDefc #45

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 2, 2018

This is an experimental pull request for prismarine-chunk that switches from ProtoDef to ProtoDefc. This pull request is not close to ready, it requires many things to be done first, most of which are documented here: ProtoDef-io/node-protodefc#7

As well, this means that this repository will only work for Node version >= 8.3 (which I have been assured is not an issue)

So, for some numbers:

  • ProtoDefc
    • 7079/duration
  • ProtoDef
    • 6273/duration

Yes. Your eyes are not deceiving you. ProtoDefc is slower, and I have tested it over 20 times. However, it is easily explained: ProtoDefc JIT compiles the protocol during the program execution. This certainly has a 1000+ms overhead. I predict that if this overhead is removed and ProtoDefc is 'warmed up', it will perform noticeably better on softwares like flying-squid. What's left:

  • Actually make sure it generates a real world and it's not just doing useless computation (yes, I haven't even tested this yet)
  • Test it against flying-squid and see if there is a noticeable improvement
  • Benchmark them fairly, i.e.: loading the compiled JS from disk (perhaps consider just bundling this JS w/ a prismarine-chunk release and add that functionality to node-protodefc)
  • Have node-protodefc ready for release

@ghost
Copy link
Author

ghost commented Jun 2, 2018

(TODO for me, format the code using standard and fix the cycle_test.js)

@rom1504
Copy link
Member

rom1504 commented Jun 2, 2018

Can't you remove the compile step from the benchmark ?

@rom1504
Copy link
Member

rom1504 commented Jun 2, 2018

If it is really slower there is little benefit...

@rom1504
Copy link
Member

rom1504 commented Jun 2, 2018

But unless it's compiling wrong, it should not be slower.
Worse case compare to manually written code.

@ghost
Copy link
Author

ghost commented Jun 2, 2018

Numbers don't seem to do this pull request any justice, so I will be posting two comparing videos that use native-voxel-worldgen, one w/ ProtoDefc (this pull request) and one w/out.
ProtoDef:
https://transfer.sh/S0DWA/voxel_worldgen_test_protodef.mp4
ProtoDefc:
https://transfer.sh/ik2IN/voxel_worldgen_test_protodefc.mp4


skylightsAddition = skyLightSent ? value.skyLight : Buffer.alloc(skyLightSize)
skylightsAddition = skyLightSent ? value.sky_light : Buffer.alloc(skyLightSize)
Copy link
Member

Choose a reason for hiding this comment

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

snake_case is wrong

Copy link
Author

Choose a reason for hiding this comment

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

ProtoDef compiler enforces it

Copy link
Member

Choose a reason for hiding this comment

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

it should transform the names to camelCase then

Copy link
Author

Choose a reason for hiding this comment

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

@@ -38,7 +38,6 @@
},
"dependencies": {
"prismarine-block": "^1.0.0",
"protodef": "^1.3.1",
Copy link
Member

Choose a reason for hiding this comment

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

well ok but shouldn't you add a dep to protodefc ?

Copy link
Author

Choose a reason for hiding this comment

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

No, it's not published we need my ProtoDefc stuff merged and my node-protodefc stuff merged. Use npm link.

@rom1504
Copy link
Member

rom1504 commented Apr 12, 2020

new version of prismarine-chunk doesn't use protodef

@rom1504 rom1504 closed this Apr 12, 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.

1 participant