Page MenuHomePhabricator

Remove lock from ConstString::GetLength
ClosedPublic

Authored by scott.smith on Apr 20 2017, 11:44 AM.

Details

Summary

ConstStrings are immutable, so there is no need to grab even a reader lock in order to read the length field.

Diff Detail

Repository
rL LLVM

Event Timeline

scott.smith created this revision.Apr 20 2017, 11:44 AM
labath accepted this revision.Apr 21 2017, 1:36 AM
labath added a subscriber: labath.

Looks good, thank you.

Out of curiosity, have you observed any performance improvements resulting from this?

This revision is now accepted and ready to land.Apr 21 2017, 1:36 AM

Looks good, thank you.

Out of curiosity, have you observed any performance improvements resulting from this?

10.2% reduction in # of instructions executed, 9.1% reduction in # of cycles, as measured by 'perf stat' in single threaded mode (I disabled TaskPool in order to get more repeatable results).

Can you commit this for me? I don't have commit access.

Thank you!

zturner added inline comments.
source/Utility/ConstString.cpp
49 ↗(On Diff #95995)

Why do we even have this function which digs into the StringMap internals rather than just calling existing StringMap member functions? Can Can we just delete GetStringMapEntryFromKeyData entirely and use StringMap::find?

labath added inline comments.Apr 25 2017, 6:22 AM
source/Utility/ConstString.cpp
49 ↗(On Diff #95995)

Can we just delete GetStringMapEntryFromKeyData entirely and use StringMap::find?

Unfortunately, I don't think that's possible. StringMap::find expects a StringRef. In order to construct that, we need to know the length of the string, and we're back where we started :(

In reality, this is doing a very different operation than find (which takes a random string and checks whether it's in the map) -- this takes a string which we know to be in the map and get its size.

It will take some rearchitecting of the ConstString class to get rid of this hack. Probably it could be fixed by ConstString storing a StringMap::iterator instead of the raw pointer. In any case, that seems out of scope of this change.

10.2% reduction in # of instructions executed, 9.1% reduction in # of cycles, as measured by 'perf stat' in single threaded mode (I disabled TaskPool in order to get more repeatable results).

That is awesome, thanks.

Can you commit this for me? I don't have commit access.

I will, let's just wait what Zachary says.

scott.smith added inline comments.Apr 26 2017, 11:03 AM
source/Utility/ConstString.cpp
49 ↗(On Diff #95995)

Probably performance. If we have to call Find, then we have to call hash, fault in the appropriate bucket, and then finally return the entry that we already have in hand. Plus we'd need the lock.

Use StringMapEntry::GetStringMapEntryFromKeyData instead of ConstString's version.

labath accepted this revision.Apr 27 2017, 1:40 AM
labath added inline comments.
source/Utility/ConstString.cpp
49 ↗(On Diff #95995)

Cool, I didn't notice that one when looking for it. I guess at this point we can just delete our copy of GetStringMapEntryFromKeyData completely and call the StringPool's version instead.

scott.smith marked 3 inline comments as done.Apr 27 2017, 9:53 AM
scott.smith added inline comments.
source/Utility/ConstString.cpp
49 ↗(On Diff #95995)

It will push a lot of lines past the 80 char limit. Do you want me to make that change? If not, can you submit this one since I do not have commit access? Thanks!

This revision was automatically updated to reflect the committed changes.
labath added inline comments.Apr 28 2017, 5:23 AM
source/Utility/ConstString.cpp
49 ↗(On Diff #95995)

Ok, nevermind, let's leave that as is for now. thanks for the patch.