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

Ensure LibUnwind::Exception struct is not atomic #8728

Merged
merged 3 commits into from Feb 5, 2020

Conversation

@waj
Copy link
Member

waj commented Jan 31, 2020

Currently any structure not containing any pointer is allocated with GC.malloc_atomic. This makes the GC faster by avoiding scan some memory regions unnecessary.

LibUnwind::Exception actually contains a pointer but it was being casted to UInt64 hidding it from the compiler.

When the ensure block for example, contains calls that ends running the GC, the exception object would be cleaned. Later the exception is re-raised but now with a dangling pointer.

The problem was easily reproduced running this code in release mode:

begin
  raise "Oh no!"
ensure
  GC.collect
end
@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Jan 31, 2020

On the arm code there is another exception_object : UInt64 but since pr_cache : ControlBlock_PrCache has nested pointer we are safe there.

@waj waj force-pushed the waj:fix/dangling-exception branch from fbde9a8 to c48484e Jan 31, 2020
@waj

This comment has been minimized.

Copy link
Member Author

waj commented Jan 31, 2020

On the arm code there is another exception_object : UInt64 but since pr_cache : ControlBlock_PrCache has nested pointer we are safe there.

Good point, so this PR might not compile in arm, right?

Copy link
Member

straight-shoota left a comment

Does the test code need to be in a separate file? It would be more clear to have the code directly in the spec.
Also is it on purpose that the file has no extension?
The spec should also be disabled for win32 because Process.run does not yet work.

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Feb 1, 2020

Another possibility is always doing malloc instead of malloc_atomic because others might be storing addresses like that. I know malloc_atomic is supposedly faster or easier for the GC but in my benchmarks that was never the case. Maybe it only matters for really big objects.

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Feb 2, 2020

I mean, anyone can use UInt64 as an address and we shouldn't just work in a wrong way. Prove me. wrong and I'll change my mind.

@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Feb 2, 2020

Pointer should be used. I see no reason UInt64 could not be replaced by Pointer. And if we ever improve to use bitmask for the GC it's better to have clearer semantics. I should not use the overconservativeness of the GC as a feature.

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Feb 2, 2020

All I'm saying is this:

  • We can't prevent users from storing object addresses as UInt64
  • I couldn't prove malloc_atomic produced faster code. I just did that optimization because it was "easy" to compute, but, as you can see, it isn't "easy".
  • If we remove this optimization the compiler is simplified because we don't need to compute that "has internal pointer" stuff.

So if we want to keep malloc_atomic I would first write a benchmark that proves that it's worth using that at the cost of having this super hard to find and debug issues.

@yxhuvud

This comment has been minimized.

Copy link
Contributor

yxhuvud commented Feb 2, 2020

While it certainly is good practice to use Pointer, there are also cases when interacting C libraries where structs can have fields defined as UInt64, but where that size where chosen INTENTIONALLY so that the user should be free to put a pointer there. Do we want to force people that implement bindings for such cases to use Void * or a more specified pointer type, or should it be possible to actually use the C type as it is defined?

I suppose an alternative solution would be to add some sort of directive that tells the compiler to treat it as a pointer. That way open-ended C bindings could be handled. Of course, this may also be nice to have in the very related area of precise GC.

Example: The user_data fields in liburing. https://github.com/axboe/liburing/blob/master/src/include/liburing/io_uring.h#L41. It works by submitting events on one queue and getting back the result on a different queue, with user_data being the way to connect the result to where it came from. In the particular case of liburing it would probably not matter how it is allocated though - there still needs to be something else preventing things from being removed as the events may be hidden away in private copies in kernel memory while the event is being processed.

(liburing is also fun to make bindings for in that it is a heavy user of static inline)

@oprypin

This comment has been minimized.

Copy link
Contributor

oprypin commented Feb 2, 2020

The "Windows CI" failure is real, btw

@waj

This comment has been minimized.

Copy link
Member Author

waj commented Feb 3, 2020

As @bcardiff said, I wouldn't relay on the conservative feature of the GC when writing bindings and other low level code. All we need to do is write proper documentation.
The performance improvement might not be noticeable on most benchmarks but I'm pretty sure it's possible to design a benchmark to demonstrate the difference. The GC will just skip scanning some memory blocks and we want every bit of CPU time we can reduce from it. So I still think using malloc_atomic when possible it's a good idea. We could add a custom attribute to attach to structures that indicates the runtime what kind of memory block has to be used. But I think the default we have right now does the right thing most of the time.

I'll check the tests running in Windows and ARM and update the PR.

waj added 2 commits Feb 3, 2020
@waj

This comment has been minimized.

Copy link
Member Author

waj commented Feb 3, 2020

Does the test code need to be in a separate file? It would be more clear to have the code directly in the spec.
Also is it on purpose that the file has no extension?

TBH, just because there was other tests doing the same thing :). We can refactor later all these tests and replace with something more elegant. The file has no extension to avoid being picked up as a spec file.

@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Feb 3, 2020

The file has no extension to avoid being picked up as a spec file.

For the record, crystal spec only recognizes files ending with _spec.cr.

@RX14
RX14 approved these changes Feb 4, 2020
@ysbaddaden

This comment has been minimized.

Copy link
Member

ysbaddaden commented Feb 5, 2020

GC.malloc_atomic shouldn't produce noticeably faster code until you allocate lots of memory with it (there can be such cases), since the GC still has to scan stacks and follow all found objects.

Note that a semi-precise GC will skip all attributes (including UInt64) and only follow actual pointers, so this patch is needed nonetheless.

@waj waj merged commit fdc88c2 into crystal-lang:master Feb 5, 2020
8 checks passed
8 checks passed
linux-job
Details
windows-job
Details
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32_std Your tests passed on CircleCI!
Details
ci/circleci: test_preview_mt Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@waj waj added this to the 0.33.0 milestone Feb 5, 2020
@waj waj deleted the waj:fix/dangling-exception branch Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.