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

GC issues on nimbus-verified-proxy #1501

Closed
vitvly opened this issue Mar 15, 2023 · 5 comments
Closed

GC issues on nimbus-verified-proxy #1501

vitvly opened this issue Mar 15, 2023 · 5 comments

Comments

@vitvly
Copy link
Member

vitvly commented Mar 15, 2023

In scope of nimbus-verified-proxy integration with status-go (status-im/status-go#3282), it turned out that various GC options that Nim provides do not work properly when invoked from Golang wrapper (https://github.com/siphiuel/lc-proxy-wrapper).

Below are the following options i've tried and respective results:

  • --gc:markAndSweep: FAIL, runtime exception "URL hostname is missing"
  • --gc:refc: FAIL, runtime SIGSEGV
  • --gc:boehm: OK, this is what being used now
  • --gc:go: FAIL, compilation error: Error: system module needs: unsureAsgnRef
  • --gc:arc/orc: FAIL, compilation error identical to the one described in Migration to v2 nim-lang/Nim#20017

Is it ok to use --gc:boehm for the time being, are there any known issues? So far i haven't encountered any during testing.

@zah
Copy link
Contributor

zah commented Mar 15, 2023

The GO run-time is scheduling the goroutines on multiple threads by default. This doesn't play well with the default Nim garbage collector which access the memory heap metadata through a thread-local variable (and generally assumes that memory allocated by a particular thread will only be access by the same thread in the future).

To work around this mismatch in expectations, I would recommend running all Nim code on a separate dedicated thread (most easily created though the functions in the Nim standard library which know how to initialize new threads with the required GC TLS metadata).

You would communicate between the GO code and the Nim code through standard IPC mechanisms, such as unix domain sockets/named pipes.

@arnetheduck
Copy link
Member

https://github.com/status-im/status-desktop/pull/9805/files#diff-0e9f723c1b6c935c40d3bbc96b65ab7632f91e6ae563bd4f5fe2103befe64094R189 implements such a trivial cross-thread mechanism for posting things onto a thread that normally runs the chronos event loop - it can of course be replaced with something more efficient in the future (which depends on some targeted updates to chronos)( but this doesn't, for practical purposes, matter in this case.

@vitvly
Copy link
Member Author

vitvly commented Apr 12, 2023

Thanks for advice! Can you please provide a bit more detail on how this can be achieved? I tried 2 things:

  1. The easier one - do runtime.LockOSThread() on Go side. Somehow predictably, it does nothing.
  2. Either plain createThread or asyncSpawn, but then the compiler complains about all kinds of things not being GC-safe. Is this expected, and the proper way to proceed is to fix these compiler errors?

@vitvly
Copy link
Member Author

vitvly commented Apr 18, 2023

Will try to provide more details about issues encountered with createThread and GC safety compiler errors.
Pushed a branch https://github.com/status-im/nimbus-eth1/tree/status_go_integration_gc_test. Tried to compile it, there were several issues related to GC-safe. Two of these fixed by commenting relevant code lines, just to get through:

  • runInThread is not GC-safe as it calls run
  • 'run' is not GC-safe as it calls 'getCLIParams'
  • 'run' is not GC-safe as it calls 'setupLogging'
    Now the following error appears:
but expected one of:
proc createThread[TArg](t: var Thread[TArg];
                       tp: proc (arg: TArg) {.thread, nimcall.}; param: TArg)
 first type mismatch at position: 2
 required type for tp: proc (arg: TArg){.nimcall, gcsafe.}
 but expression 'runInThread' is of type: proc (conf: VerifiedProxyConf){.closure, gcsafe, locks: <unknown>.}
 Calling convention mismatch: got '{.closure.}', but expected '{.nimcall.}'.```

@vitvly
Copy link
Member Author

vitvly commented Nov 3, 2023

Fixed by using context pointers. Superseded by #1882

@vitvly vitvly closed this as completed Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants