NimYAML compatibility with ARC/ORC.

[ghost]

Hello! I've been testing a lot of Nim libraries with ARC/ORC, and NimYAML is one of them :) After nim-lang/Nim#15052 and some manual patching it seems to work fine with --gc:orc --sinkInference:off (sink inference can also be enabled if you add two {.nosinks.}`.

Would you be interested in somehow adding these patches to NimYAML without breaking compatibility with older Nim versions? FYI - destructors are mapped to finalizers (but I think only on 1.2+), so we'll need to have more when branching.

The patch itself is https://gist.github.com/Yardanico/e03d29b4db56366543bbb01d935daa62 (you can ignore nosinks parts - they don't need to be added if you compile with --sinkInference:off). The main problem I solved there is that new Nim runtime (with ARC/destructors) can't map multiple finalizers, because destructors are type-bound. So I had to create two additional bool fields (I suppose we could replace them with just one isLexer, and check if it's true or false). With this patch NimYAML passed all tests for me with both refc (default GC) and orc on latest devel.

[flyx]

ARC is a bad idea for the YAML DOM since that can contain cycles. Generally I am not up-to-date with those newish Nim GC systems, so I can't quickly assess its impact on native serialization.

Did you run tests with valgrind to check whether memory is properly managed?

[ghost]

ARC is a bad idea for the YAML DOM since that can contain cycles. Generally I am not up-to-date with those newish Nim GC systems, so I can't quickly assess its impact on native serialization.

Did you run tests with valgrind to check whether memory is properly managed?

Well there's ORC as well, which is ARC with cycles. I ran Valgrind and had some memory leaks, but didn't try to investigate them yet. Is there any reason you used raw pointers for storing the source object?

[flyx]

The source object has a long and complex history. If I recall correctly:

• Initially it was a BaseLexer.
• At some point, I tried to support the JS backend. BaseLexer wasn't supported there so I added StringSource.
• To maximize performance, I wanted to have StringSource supported for the default target as well (in-memory strings need no caching).
• A dispatching call for every character didn't seem good performance-wise, so instead, I did the pointer hack along with internal generics and casts.
• The idea was to eventually create an own BaseLexer to support UTF-16 and UTF-32 as required by the YAML spec and enable that lexer to initialize with a given string, so that I can throw away the StringSource for non-JS.

The last step never happened and so the temporary hack went the way of all temporary hacks and has been in the code base for years :).

[ghost]

@flyx FYI - lexbase works with Nim's JS backend on devel and that will be in 1.4 :)
EDIT: Ah, actually it might already work in 1.2

[flyx]

As for the original topic: I would like NimYAML to fail at compile time if generated code is not guaranteed to not leak memory. This would mean that e.g. the DOM API should fail with a compile-time error if compiled with --gc:arc, while --gc:orc would be fine. For native serialization, I think it's okay to burden the user with checking whether --gc:arc is safe, so this should compile with both options.

If there are any internal memory leaks using ARC/ORC, those should be addressed before officially supporting these GCs.

I will look into removing the source pointer in favor for BaseLexer when I have time.

[ghost]

@flyx yeah, it's totally fine, ORC will (probably) be the new default GC in 1.4, not ARC.
As for checking at compile-time:

when defined(gcArc) and not defined(gcOrc):
  {.error: "NimYAML only supports ORC because ARC can't deal with cycles".}

[ghost]

@flyx also, an update - ORC will not be the default in 1.4, but I hope that we manage to get NimYAML to support it before ORC is made default in future Nim releases :)

[flyx]

A lexer/parser rewrite has landed in devel which removes the raw pointer usage during parsing. Raw pointers are still used to resolve aliases during deserialization and I don't really see a way around that. The question here is whether ORC will properly count references when I cast ref values to pointer and back.

Regarding your patch, only removing not nil from YamlNode still seems relevant. I dislike this patch since it removes a relevant constraint. Can you elaborate on why it's needed?

[flyx]

I disabled the DOM API for ARC. Current head now compiles and tests green with both ORC and ARC, the latter without the DOM API tests. Since I rewrote the parser, that part of your patch is obsolete. I am unsure whether the part of your patch for dom.nim still has relevance since the code builds without that patch.

[ghost]

Patch to dom.nim is not needed anymore since sink inference is disabled for user code now. You can still add the changes if you want to make sure that if users want to use it with your lib - it works, but I don't think that it's such a big deal. (Try compiling with --sinkInference:off}