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

Abstract LibGC usages. Enable none garbage collector. #5314

Merged

Conversation

ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Nov 20, 2017

All direct calls do LibGC are now abstracted under the GC module. The null and boehm garbage collectors were updated to follow the new API (the null GC compiles again). The __crystal_malloc internal functions were also abstracted to call into GC.malloc where implementation / bindings are expected to be.

Also introduces a GC_NULL compilation flag to select the null garbage collection instead of Boehm GC.

This patch allows experimenting with alternative or custom garbage collectors.

@ysbaddaden
Copy link
Contributor Author

(now nobody will guess whatever I've been getting my hands dirty with these past few weeks)

Copy link
Contributor

@RX14 RX14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps just a small spec that checks that hello world compiles and runs using -Dnull_gc?

src/gc.cr Outdated
end
end

{% if flag?(:GC_NULL)%}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically flags so far have been lowercase so far (without_openssl, etc), why not null_gc?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe -Dno_gc?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's the case, maybe we should rename gc/null to gc/none?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn’t use “no gc”. Seems more like a toggle to switch to a c++ like memory management to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Habit? Too much C reading? Constant-like looking? Anyway, can change to whatever.

src/gc.cr Outdated

# :nodoc:
fun __crystal_malloc_atomic(size : UInt32) : Void*
GC.malloc(LibC::SizeT.new(size))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point if __crystal_malloc_atomic & __crystal_malloc have the same implementation?
It doesn't look right.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, these should use GC.malloc_atomic.

end
{% end %}

GC.malloc(LibC::SizeT.new(size))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto regarding atomic

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bcardiff I think this was addressed in latest change, can you check again? Thank you.

src/gc.cr Outdated
end
end

{% if flag?(:GC_NULL)%}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn’t use “no gc”. Seems more like a toggle to switch to a c++ like memory management to me.

@ysbaddaden ysbaddaden force-pushed the core-abstract-garbage-collector branch 2 times, most recently from 8c399c4 to 0bfd178 Compare November 20, 2017 14:17
@ysbaddaden ysbaddaden changed the title Abstract LibGC usages. Reenable null garbage collector. Abstract LibGC usages. Enable none garbage collector. Nov 20, 2017
@ysbaddaden
Copy link
Contributor Author

Fixed thread issue in specs. Fixed atomic malloc calls. Renamed GC_NULL flag to gc_none.

@RX14
Copy link
Contributor

RX14 commented Nov 20, 2017

@ysbaddaden please make fixup! commits so I can review just the changes!

src/gc.cr Outdated
end
end

{% if flag?(:gc_none)%}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing space before %}

@ysbaddaden ysbaddaden force-pushed the core-abstract-garbage-collector branch from 0bfd178 to 355d4d1 Compare November 20, 2017 17:06
All direct calls do LibGC are now abstracted under the GC module;
the null GC was updated to follow the API. The `__crystal_malloc`
collection of internal functions were also abstracted to call into
the `GC.malloc` implementations.

Also introduces a `GC_NULL` compilation flag to select the null GC
instead of Boehm GC, which enables experiments to implement or use
alternative garbage collectors.
@ysbaddaden ysbaddaden force-pushed the core-abstract-garbage-collector branch from 355d4d1 to c08c7ad Compare November 20, 2017 17:07
@RX14 RX14 merged commit e687dee into crystal-lang:master Nov 22, 2017
@ysbaddaden ysbaddaden deleted the core-abstract-garbage-collector branch November 22, 2017 13:11
@RX14 RX14 added this to the Next milestone Dec 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants