Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Comments

GC: make gcLock.lock/unlock inlinable#1178

Closed
rainers wants to merge 1 commit intodlang:masterfrom
rainers:gc_lock_final
Closed

GC: make gcLock.lock/unlock inlinable#1178
rainers wants to merge 1 commit intodlang:masterfrom
rainers:gc_lock_final

Conversation

@rainers
Copy link
Member

@rainers rainers commented Feb 18, 2015

While looking at the disassembly for #1147 I noticed that lock/unlock on GCMutex are still called through the virtual function table, even though GCMutex is declared as final.
Calling Mutex.lock/unlock through a final method in GCMutex fails to inline, so I made final versions of the methods in Mutex.
In addition, making gcLock a property instead of a pointer avoids an indirection.

This change caused a 0-5% speedup for the benchmarks.

Copy link
Member

Choose a reason for hiding this comment

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

That's fairly ugly to change the API, and you can't use package protection here.
Maybe we just copy over the implementation to the GC?

Copy link
Member

Choose a reason for hiding this comment

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

And someone should fix the DMD bug, is it reported in Bugzilla?

Copy link
Member Author

Choose a reason for hiding this comment

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

And someone should fix the DMD bug, is it reported in Bugzilla?

I added https://issues.dlang.org/show_bug.cgi?id=14211

@MartinNowak
Copy link
Member

Nice

@MartinNowak
Copy link
Member

Yeah, a struct with the implementation from core sync mutex would be best, because I want to replace it with a spinlock anyhow.

@rainers
Copy link
Member Author

rainers commented Feb 20, 2015

Yeah, a struct with the implementation from core sync mutex would be best, because I want to replace it with a spinlock anyhow.

Good idea, done that.

@MartinNowak
Copy link
Member

Looks like destroying the mutex fails because of an unbalanced calls to unlock.
c++ - pthread_mutex_destroy: why is it returning EBUSY?
Happens for rt.lifetime which tests FinalizeErrors. As we've learned during the spinlock pull, those don't perform proper cleanup in the GC. Maybe we should move the FinalizeError tests to an external test program that actually dies correctly.

@rainers
Copy link
Member Author

rainers commented Feb 28, 2015

Thanks for the info. I'm not sure why this works with the original GCMutex, though.

We could also add some minimalistic cleanup in case of a finalization error:

  • add try/catch to Gcx.sweep
  • cleanup code: running = false; gcLock.unlock(); throw ex;

@MartinNowak MartinNowak added the GC garbage collector label Mar 7, 2015
@rainers
Copy link
Member Author

rainers commented Mar 14, 2015

dlang/dmd#4427 added the devirtualization, so this PR is obsolete.

@rainers rainers closed this Mar 14, 2015
@MartinNowak
Copy link
Member

Nice

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

GC garbage collector

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants