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

simplify extccomp.nim json logic via jsonutils; fix #18084 #18100

Merged
merged 11 commits into from
Jun 3, 2021

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented May 26, 2021

@timotheecour timotheecour marked this pull request as ready for review May 26, 2021 08:00
@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label May 26, 2021
@@ -14,7 +14,7 @@

import ropes, platform, condsyms, options, msgs, lineinfos, pathutils

import os, strutils, osproc, std/sha1, streams, sequtils, times, strtabs, json
import std/[os, strutils, osproc, sha1, streams, sequtils, times, strtabs, json, jsonutils, sugar]
Copy link
Member

Choose a reason for hiding this comment

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

No. If you want to improve this, give us a "jsonbuilder" module first. One that doesn't repeat json's mistakes.

Copy link
Member Author

@timotheecour timotheecour Jun 2, 2021

Choose a reason for hiding this comment

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

@Araq are you referring to timotheecour#745 (serialize/deserialize directly from string instead of from intermediate JsonNode) or to something else? If so, yes I agree timotheecour#745 is needed down the line for performance in similar cases but it would be a 1 liner change on top of this PR once that feature lands. If you're worried about performance I can make a benchmark but I don't see a reason why this PR would create any meaningful overhead.

the name jsonbuilder sounds like it'd be instead similar to the json.%* operator (%* {"age":35, "pi":3.1415}), but that's worse than what I'm doing as there's no spec/type-safety and serialization/deserialization would need more synchronization, unlike this PR with BuildCache (which is analog to a protobuf proto)

this PR is a big code simplification as evidenced by the diff

@Araq
Copy link
Member

Araq commented Jun 3, 2021

serialize/deserialize directly from string instead of from intermediate JsonNode)

Yes, that's my point.

If so, yes I agree timotheecour#745 is needed down the line for performance in similar cases but it would be a 1 liner change on top of this PR once that feature lands.

I fail to see how.

the name jsonbuilder sounds like it'd be instead similar to the json.%* operator (%* {"age":35, "pi":3.1415}), but that's worse than what I'm doing as there's no spec/type-safety and serialization/deserialization would need more synchronization

I don't understand but jsonbuilder's API is not outlined anywhere so it's hard to communicate.

@timotheecour
Copy link
Member Author

timotheecour commented Jun 3, 2021

it would be a 1 liner change on top of this PR once that feature lands.

I fail to see how.

once timotheecour#745 lands, the code in this PR can be changed as follows:

conf.jsonBuildFile.string.writeFile(bcache.toJson.pretty)
...
bcache.fromJson(jsonFile.string.parseFile)

=>

conf.jsonBuildFile.string.writeFile(bcache.toJsonStr(pretty = true))
...
bcache.fromJsonStr(jsonFile.string.readFile) # or directly from a stream to avoid `readFile`

toJsonStr is defined (briefly) in timotheecour#745

in the meantime, there should be no performance penalty resulting from this PR even in absence of toJsonStr, fromJsonStr

@timotheecour
Copy link
Member Author

timotheecour commented Jun 3, 2021

there should be no performance penalty resulting from this PR

and indeed, here are timing results:
if i instrument (and compile nim with -d:release as is usual) writeJsonBuildInstructions with let t = cpuTime() at beginning and echo cpuTime() - t at end, here are results (best of 3 runs):

before PR

nim c compiler/nim 0.0006
nim c -f compiler/nim 0.013 # more expensive because it calls getCompileCFileCmd in this case

after PR

nim c compiler/nim 0.0006
nim c -f compiler/nim 0.011 # ditto

=> no performance drop as a result of this PR, not that there's anything surprising to that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review (please take another look): ready for next review round
Projects
None yet
Development

Successfully merging this pull request may close these issues.

-d:nimBetterRun (or nim r) should recompile if set of config files change
2 participants