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

double JSON.stringify/parse when talking to extension host process #2088

Closed
jrieken opened this issue Jan 18, 2016 · 9 comments
Closed

double JSON.stringify/parse when talking to extension host process #2088

jrieken opened this issue Jan 18, 2016 · 9 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Jan 18, 2016

the default IPC mechanism in nodejs already does JSON.stringify and JSON.parse before sending and after receiving messages. Maybe we gain a little bit of performance we don't do right before/after.

@jrieken jrieken added the bug Issue identified by VS Code Team member as probable bug label Jan 18, 2016
@jrieken
Copy link
Member Author

jrieken commented Jan 18, 2016

unsure if this @alexandrudima or @bpasero

@weinand
Copy link
Contributor

weinand commented Jan 18, 2016

@alexandrudima please reassign if @bpasero is the author of this code.

@bpasero
Copy link
Member

bpasero commented Jan 18, 2016

I think we are doing more than just JSON.stringify/parse though, there is code that knows how to marshall classes.

@jrieken
Copy link
Member Author

jrieken commented Jan 18, 2016

@alexdima
Copy link
Member

I was doing the extra JSON stringify/parse intentionally back when the extension host was using electron's IPC which had bugs around serializing the same object twice. e.g.:

var obj1 = { hello: 'world' }
var arr = [obj1, obj1];

it would arrive on the other side as [obj1, null].

But I think this is no longer mandatory given @bpasero 's refactoring it to a forked process.

@alexdima alexdima assigned bpasero and unassigned alexdima Jan 19, 2016
@bpasero bpasero assigned alexdima and unassigned bpasero Jan 19, 2016
@bpasero
Copy link
Member

bpasero commented Jan 19, 2016

@alexandrudima I am not feeling comfortable making any changes to marshalling.ts, I did not touch this when I created the forked process. please go ahead and see what happens when you just take out the serialization.

@jrieken jrieken added this to the Jan 2016 milestone Jan 19, 2016
@jrieken jrieken assigned jrieken and unassigned alexdima Jan 19, 2016
@jrieken
Copy link
Member Author

jrieken commented Jan 19, 2016

I'll take on the challenge

@bpasero
Copy link
Member

bpasero commented Jan 19, 2016

🍦

@alexdima
Copy link
Member

Thanks! ❤️

@jrieken jrieken assigned alexdima and unassigned jrieken Jan 26, 2016
@bpasero bpasero assigned weinand and aeschli and unassigned alexdima and weinand Jan 29, 2016
@aeschli aeschli assigned bpasero and unassigned aeschli Jan 29, 2016
@bpasero bpasero added the verified Verification succeeded label Jan 29, 2016
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

5 participants