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

Proposal to remove the .gcsafe effect from Nim #142

Closed
Araq opened this issue Mar 10, 2019 · 17 comments
Closed

Proposal to remove the .gcsafe effect from Nim #142

Araq opened this issue Mar 10, 2019 · 17 comments

Comments

@Araq
Copy link
Member

Araq commented Mar 10, 2019

The one "effect" that most people encounter when starting with Nim is the so called "GC safety", .gcsafe. This effect ensures that globals are not used to construct data structures that span multiple threadlocal heaps. There also exists a {.gcsafe.}: block construct to override the compiler's idea of how globals are really used at runtime. If the global is only accessed from the main thread, the program is still safe. This proposal argues that this runtime check is the only really required check and the language becomes simpler to use if we remove the .gcsafe effect (but not the block!)

So here is how it can work: Read or write accesses to globals that use GC'ed memory have to be put into a {.gcsafe.}: block:

var s: seq[string]

proc main =
  {.gcsafe.}:
    echo s

This block is translated by the compiler into:

var s: seq[string]

proc main =
  doAssert isMainThread()
  echo s

The .gcsafe effect for routines and proc types will continue to compile for a migration period but the annotation means nothing then.

Q&A

  1. Why not compile this assertion into the code automatically?

Because the programmer needs to be aware of this restriction.

  1. Why not name it {.assertOnMainThread.} instead?

That's something we could do.

  1. Why not compile these globals to thread local variables instead?

Because thread local storage currently cannot be initialized explicitly. Also it would break the consistency with global variables that do not use GC'ed memory. Wether a variable is threadlocal or global depends on the problem at hand and the programmer must be aware of the differences.

  1. Isn't replacing a compile-time check by a runtime check a bad idea?

In general yes but I think in this case it is justified. The .gcsafe effect bubbles up and all callbacks in the libraries end up with this restriction just to make the compiler happy. In the end people write {.gcsafe.}: use myGlobal in their application code anyway in order to get the job done. And then they don't use any runtime check whatsoever.

@Araq Araq changed the title Proposal to remove the .gcsafe effect from Nim Proposal to remove the .gcsafe effect from Nim Mar 10, 2019
@ryukoposting
Copy link

Almost anyone who has written software professionally has at least one horror story about a memory bug suddenly blowing up some application that had been in production for months. That stuff isn't fun to fix. Sure, you can save time using a language that is "easier to write," but if it means you're going to have to come back to that code later to fix bugs that could have been prevented with good static analysis, then what's the point?

Nim is already very easy to write, and it makes very few safety-related compromises. .gcsafe provides an extra bit of protection against one of the nastiest kinds of bugs. The average Nim program doesn't have {.gcsafe.}: blocks all over the place, so it isn't placing an overwhelming burden on Nim programmers, either.

I'd be curious to know about others' experience with .gcsafe. In my experience, .gcsafe rarely causes errors. When it does cause errors, it's caused by a legitimate bug in the my code. It's helpful, and it doesn't make the language harder to use.

@stefantalpalaru
Copy link

doAssert isMainThread()

That's not enough. We also need access from other threads to global variables guarded with locks.

@Araq
Copy link
Member Author

Araq commented Mar 11, 2019

That's not enough. We also need access from other threads to global variables guarded with locks.

That's not possible with the thread local heaps and even innocent "readonly" accesses could under the hood trigger a GC write barrier which is allowed to be thread-unsafe and racy. Let's please stick to the topic.

@pgkos
Copy link

pgkos commented Mar 11, 2019

Consider the following case:

proc someImportedProcThatSchedulesCallbackOnMainThread(callback: proc) {.importc.}

proc accessesGcMemory =
  # some accesses / writes to gc memory here ...

proc runsNotOnMainThread(callback: proc) =
  # 'callback' is set to 'accessesGcMemory'
  {.gcsafe.}
    someImportedProcThatSchedulesCallbackOnMainThread(callback)

Currently, without the {.gcsafe.} block this would fail with an "indirect call".

Would such a case still be supported?

@Araq
Copy link
Member Author

Araq commented Mar 11, 2019

Would such a case still be supported?

Yes but the {.gcsafe.} block moves into accessesGcMemory.

@zah
Copy link
Member

zah commented Mar 11, 2019

Once strings and sequences are no longer considered garbage collected objects, the need for global gc-unsafe variables will be very rare and the gcsafe-related problems will be much less pronounced. I don't think we need to cripple the safety of the language to solve this potential non-problem.

To make the life of the user simpler, gcsafe can be considered the default for proc types. We can rename the effect to gcunsafe.

@Araq
Copy link
Member Author

Araq commented Mar 11, 2019

Once strings and sequences are no longer considered garbage collected objects, the need for global gc-unsafe variables will be very rare and the gcsafe-related problems will be much less pronounced.

But that is not true at all, the .gcsafe effect would still exist and affect callback types etc, no matter if you use globals with GC'ed memory and no matter what "GC'ed memory" ends up to mean. That's the point of my proposal, it's an overly pessimistic design for the gains it gives us.

To make the life of the user simpler, gcsafe can be considered the default for proc types. We can rename the effect to gcunsafe.

Yes, that would also attack the problem. But currently all effects in Nim are opt-in, not opt-out, so it would be an inconsistency.

@zah
Copy link
Member

zah commented Mar 11, 2019

Yes, that would also attack the problem. But currently all effects in Nim are opt-in, not opt-out, so it would be an inconsistency.

Hrm, the effect system in general is much more useful for specifying the absence of something:

  1. This async loop is not invoking blocking operations
  2. This algorithm is not performing memory allocations
  3. This code doesn't throw any exceptions besides FooError
  4. This code is not using GC-unsafe variables

This line of thinking makes gcsafe the inconsistent one, not gcunsafe, but perhaps you mean that none of these restrictions are imposed on the proc types by default?

@Araq
Copy link
Member Author

Araq commented Mar 11, 2019

but perhaps you mean that none of these restrictions are imposed on the proc types by default?

Yup.

@dom96
Copy link
Contributor

dom96 commented Mar 13, 2019

I'm fine with this but isn't your plan to get rid of these thread-local heaps as well?

@Araq
Copy link
Member Author

Araq commented Mar 16, 2019

I'm fine with this but isn't your plan to get rid of these thread-local heaps as well?

Yes indeed but let's not make the perfect the enemy of the good.

@yglukhov
Copy link
Member

There's only one thing I use {.gcsafe.}: myBlock for, and that's accessing global GC-backed data from a background thread. The proposal seems to break this for me.

@ghost
Copy link

ghost commented Jul 26, 2019

I'd be curious to know about others' experience with .gcsafe. In my experience, .gcsafe rarely causes errors. When it does cause errors, it's caused by a legitimate bug in the my code. It's helpful, and it doesn't make the language harder to use.

After playing around with Nim I very quickly (during the first day) ran into {.gcsafe.} while using asyncdispatch's addTimer and the AsyncHttpServer. I had to read up on why {.gcsafe.} is needed, moved my globals to threadvars and I was pretty much done.

I don't think it's too bad of an experience, but I think it would be nice if the {.gcsafe.} in async code was only required in multi-threaded applications, since it doesn't give any additional protection when the application is single-threaded.

@dom96
Copy link
Contributor

dom96 commented Jul 27, 2019

code was only required in multi-threaded applications, since it doesn't give any additional protection when the application is single-threaded.

The idea is to make all code threading-ready, but I strongly think that's not necessary so I agree with you completely here.

@snej
Copy link

snej commented Jun 16, 2020

After playing around with Nim I very quickly (during the first day) ran into {.gcsafe.} while using asyncdispatch's addTimer and the AsyncHttpServer. I had to read up on why {.gcsafe.} is needed, moved my globals to threadvars and I was pretty much done.

I also hit gcsafe errors when first trying to work with asynchttpserver. In my case I have declared no globals, and the compiler errors point to a global synthesized by the compiler inside an async macro, so I'm stumped. ¯\(ツ)/¯ (link to forum thread)

@mildred
Copy link

mildred commented Nov 19, 2020

There is an issue appearing in zevv/npeg#28

npeg is a gc safe library, but the APi accepts callbacks. it is perfectly legitimate to use the API in GC unsafe code, in which case you will want to use GC unsafe callbacks. In gc safe code, you'll want the API to be gcsafe and you'll provide gc safe callbacks.

However, the library author has to make a choice in either being gcsafe or not. When it accepts callbacks, if the callback are annotated gcsafe, then the whole library is gcsafe, but your callback must obey gcsafe. Else, the whole library is gc unsafe.

There should be a way to tell that the code is safe but may accept unsafe callbacks.

@Araq
Copy link
Member Author

Araq commented May 16, 2021

There is a better way to do this once we ship --gc:orc as the default, closing.

@Araq Araq closed this as completed May 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants