Improve JSON serialisation of strtabs #14549

PMunch · 2020-06-02T15:55:29Z

This creates a more compact serialisation of strtabs that is more in
line with the normal tables.

This creates a more compact serialisation of strtabs that is more in line with the normal tables.

alaviss · 2020-06-02T21:29:13Z

lib/pure/json.nim

@@ -353,6 +353,14 @@ proc `[]=`*(obj: JsonNode, key: string, val: JsonNode) {.inline.} =
  assert(obj.kind == JObject)
  obj.fields[key] = val

+proc `%`*(table: StringTableRef): JsonNode =


Shouldn't this be done inside strtabs itself? The module can make use of external % implementation afaik (and if it doesn't, we can make it do so)

Well the json module already contains the implementation for Table and OrderedTable so it just felt more at home along with them. But I don't object to moving it.

Well it does save us from having to export the mode field :P

True, but at least this field should be readable through a template anyways..

Araq · 2020-06-03T15:58:28Z

json.nim should use some concept like TableLike that strtabs fullfills.

PMunch · 2020-06-03T17:56:11Z

Not sure that would work as strtabs still needs to store its mode..

Araq · 2020-06-04T08:41:57Z

Then maybe it should be a Serializable concept?

PMunch · 2020-06-04T09:07:32Z

Well they are serialiseable as they are now, aren't most things serialiseable? The issue here was just that strtabs stored the internal sequence of data which meant a lot of empty fields. Just have a look at one of the old logs (warning, takes a while to load). Most of that log is just {"Field0": null, "Field1": null}.

Araq · 2020-06-04T09:11:56Z

Sure, my major gripe is a json.nim that imports all the stdlib collections, there should be a better way. At least it's better than everything depending on json though...

alaviss · 2020-06-04T19:45:21Z

Isn't just implementing all of these functions in strtabs itself will solve this issue? Or am I missing something here?

PMunch · 2020-06-04T20:03:02Z

By that logic every module would end up depending on the JSON module, which isn't great. Is there a way to check if the importer imports another module as well?

timotheecour · 2020-06-05T01:44:33Z

a simple solution is a new module std/jsonutils that imports std/[json,strtabs] + maybe other and provides json serialization for those; you can either expose mode (should be done regardless anyways) or use fields trick to get the mode.
a better solution is via toJson, jsonTo, json (de)serialization for custom types; remove dependency on strtabs thanks to a hooking mechanism #14563; it avoids need for a 3rd module, and avoids json importing strtabs or vice versa, see test:

let t = {"name": "John", "city": "Monaco"}.newStringTable
doAssert $t.toJson2 == """{"mode":"modeCaseSensitive","city":"Monaco","name":"John"}"""

and when every symbol becomes 1st class; defines 0-cost lambda and aliases; generics/iterators/templates/etc can be passed to any routine #11992 materializes, it'll improve things further as it'll allow passing arbitrary symbols (eg un-instantiated generic, or template or iterator...) to other symbol, so that it would be generalizable to work with like CustomTable[K,V] in customtable.nim without json having to be aware of customtable or vice versa

PMunch · 2020-06-05T06:41:31Z

A new module would still need to import everything that should have special serialisations. It is better than forcing everyone who imports the json module to also import them, but it's still not great.
This doesn't really work as I explained in the linked draft. The problem is that you can't have "mode" as a key in the table any longer as it would shadow the "mode" of the table itself.

Not sure how feasable it is, but I believe the best option is a pragma that conditionally checks if the importer has imported another module, and if so then define procedures that have this module available to them. This would mean the code could be moved into strtabs and look a bit something like this:

proc `%`*(table: StringTableRef): JsonNode {.withImport: json.} =
## Generic constructor for JSON data. Creates a new ``JObject JsonNode``.
  result = newJObject()
  result["mode"] = %($table.mode)
  var data = newJObject()
  for k, v in table: data[k] = %v
  result["data"] = data

timotheecour · 2020-06-05T07:33:33Z

This doesn't really work as I explained in the linked draft. The problem is that you can't have "mode" as a key in the table any longer as it would shadow the "mode" of the table itself.

that's now fixed see #14563 (comment) (and again, with #11992 you'd pass a single symbol instead of having to pass 3 procs)

PMunch · 2020-06-05T08:45:09Z

Would it be possible to merge this for now so that strtabs could be serialised better, and then create an RFC where we can discuss the various ways of solving the json <-> strtabs (or more generally the serialiser <-> serialisee) relationship?

Araq · 2020-06-05T08:54:30Z

Ok.

timotheecour · 2020-06-05T09:52:14Z

after this merge, merely importing json results in the following extra modules:

Hint: strtabs [Processing]
Hint: os [Processing]
Hint: pathnorm [Processing]
Hint: osseps [Processing]
Hint: posix [Processing]
Hint: times [Processing]

I don't think that's good, given that os and times are in particular heavy dependencies.
Especially when I showed there was an alternative way #14563

json should be light and unrelated to any OS specific stuff, but is now a kitchen sink of dependencies; here's the total imports for import json:

Hint: system [Processing]
Hint: widestrs [Processing]
Hint: io [Processing]
Hint: t10888 [Processing]
Hint: json [Processing]
Hint: hashes [Processing]
Hint: tables [Processing]
Hint: math [Processing]
Hint: bitops [Processing]
Hint: macros [Processing]
Hint: algorithm [Processing]
Hint: strtabs [Processing]
Hint: strutils [Processing]
Hint: parseutils [Processing]
Hint: unicode [Processing]
Hint: os [Processing]
Hint: pathnorm [Processing]
Hint: osseps [Processing]
Hint: posix [Processing]
Hint: times [Processing]
Hint: options [Processing]
Hint: typetraits [Processing]
Hint: lexbase [Processing]
Hint: streams [Processing]
Hint: parsejson [Processing]

ghost · 2020-06-05T11:07:09Z

@timotheecour but we have dead code elimination anyway, so I don't see it being that bad

PMunch · 2020-06-05T11:10:30Z

Yes, the extra imports are bad, no-one is arguing the opposite (except Yardanico it seems :P). But this should be discussed elsewhere and implemented properly, however this has little bearing on strtabs in particular.

One reason it's bad: if you want to bundle NimScript into your program and want to create a custom stdlib you'd have to rip things out of files in order to bring the size down. It's a small use-case, but in general I think it would be a good idea to keep the interconnectedness of libraries to a minimum.

This reverts commit 7cb4ef2.

disruptek · 2020-06-06T02:39:14Z

See what you think of my approach here: https://github.com/disruptek/jason

timotheecour · 2020-06-06T06:10:09Z

@Yardanico

@timotheecour but we have dead code elimination anyway, so I don't see it being that bad

it still needs to be parsed and is not cheap, eg look at this: #14179
simply adding dependency on os (without using any symbols) yields a 30x slowdown in that case

and it can lead to cyclic dependency issues

This reverts commit 7cb4ef2.

…ependency on strtabs thanks to a hooking mechanism (#14563) * json custom serialization; application for strtabs * serialize using nesting * make toJson more feature complete * add since * Revert "Improve JSON serialisation of strtabs (#14549)" This reverts commit 7cb4ef2. * better approach via mixin * toJson, jsonTo * fix test * address comments * move to jsonutils * doc * cleanups * also test for js * also test for vm

Improve JSON serialisation of strtabs

f3be709

This creates a more compact serialisation of strtabs that is more in line with the normal tables.

PMunch mentioned this pull request Jun 2, 2020

Modernise NimBot nim-lang/nimbot#15

Merged

alaviss reviewed Jun 2, 2020

View reviewed changes

timotheecour mentioned this pull request Jun 5, 2020

toJson, jsonTo, json (de)serialization for custom types; remove dependency on strtabs thanks to a hooking mechanism #14563

Merged

Araq merged commit 7cb4ef2 into nim-lang:devel Jun 5, 2020

timotheecour added a commit to timotheecour/Nim that referenced this pull request Jun 6, 2020

Revert "Improve JSON serialisation of strtabs (nim-lang#14549)"

7578ce5

This reverts commit 7cb4ef2.

timotheecour added a commit to timotheecour/Nim that referenced this pull request Jun 6, 2020

Revert "Improve JSON serialisation of strtabs (nim-lang#14549)"

4fa6896

This reverts commit 7cb4ef2.

timotheecour added a commit to timotheecour/Nim that referenced this pull request Jun 7, 2020

Revert "Improve JSON serialisation of strtabs (nim-lang#14549)"

e36c23e

This reverts commit 7cb4ef2.

c-blake mentioned this pull request Aug 9, 2020

Add some enhancements to jsonutils.nim #15133

Merged

timotheecour mentioned this pull request Dec 27, 2020

Update json.nim #15420

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve JSON serialisation of strtabs #14549

Improve JSON serialisation of strtabs #14549

PMunch commented Jun 2, 2020

alaviss Jun 2, 2020

PMunch Jun 2, 2020

alaviss Jun 2, 2020

PMunch Jun 2, 2020

Araq commented Jun 3, 2020

PMunch commented Jun 3, 2020

Araq commented Jun 4, 2020

PMunch commented Jun 4, 2020 •

edited

Loading

Araq commented Jun 4, 2020

alaviss commented Jun 4, 2020

PMunch commented Jun 4, 2020 •

edited

Loading

timotheecour commented Jun 5, 2020 •

edited

Loading

PMunch commented Jun 5, 2020

timotheecour commented Jun 5, 2020 •

edited

Loading

PMunch commented Jun 5, 2020

Araq commented Jun 5, 2020

timotheecour commented Jun 5, 2020 •

edited

Loading

ghost commented Jun 5, 2020

PMunch commented Jun 5, 2020 •

edited

Loading

disruptek commented Jun 6, 2020

timotheecour commented Jun 6, 2020 •

edited

Loading

Improve JSON serialisation of strtabs #14549

Improve JSON serialisation of strtabs #14549

Conversation

PMunch commented Jun 2, 2020

alaviss Jun 2, 2020

Choose a reason for hiding this comment

PMunch Jun 2, 2020

Choose a reason for hiding this comment

alaviss Jun 2, 2020

Choose a reason for hiding this comment

PMunch Jun 2, 2020

Choose a reason for hiding this comment

Araq commented Jun 3, 2020

PMunch commented Jun 3, 2020

Araq commented Jun 4, 2020

PMunch commented Jun 4, 2020 • edited Loading

Araq commented Jun 4, 2020

alaviss commented Jun 4, 2020

PMunch commented Jun 4, 2020 • edited Loading

timotheecour commented Jun 5, 2020 • edited Loading

PMunch commented Jun 5, 2020

timotheecour commented Jun 5, 2020 • edited Loading

PMunch commented Jun 5, 2020

Araq commented Jun 5, 2020

timotheecour commented Jun 5, 2020 • edited Loading

ghost commented Jun 5, 2020

PMunch commented Jun 5, 2020 • edited Loading

disruptek commented Jun 6, 2020

timotheecour commented Jun 6, 2020 • edited Loading

PMunch commented Jun 4, 2020 •

edited

Loading

PMunch commented Jun 4, 2020 •

edited

Loading

timotheecour commented Jun 5, 2020 •

edited

Loading

timotheecour commented Jun 5, 2020 •

edited

Loading

timotheecour commented Jun 5, 2020 •

edited

Loading

PMunch commented Jun 5, 2020 •

edited

Loading

timotheecour commented Jun 6, 2020 •

edited

Loading