AffinitySupport.getThreadId in VanillaChronicle

[Ross Black]

Hi,

I am using VanillaChronicle.  All my code ran fine during development, but fails on production machines.  I discovered that this is because the thread id is larger than expected.

VanillaChronicle.java
--
                appenderThreadId = AffinitySupport.getThreadId();
                assert (appenderThreadId & 0xFFFF) == appenderThreadId : "appenderThreadId: " + appenderThreadId;
--

All of the thread ids are currently in the 500000 range (e.g. 571538 = 0x8b892), but the Chronicle code assumes that they are 32bit values.

The machine is running ubuntu:
Linux ihz41 3.10.3-2-generic #14~abx2 SMP Mon Aug 5 02:41:52 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux
Distributor ID:    Ubuntu
Description:    Ubuntu 12.04.2 LTS
Release:    12.04
Codename:    precise


Should the VanillaChronicle code simply create the thread id by masking with 0xFFFF instead of asserting, or is there some other solution?


Thanks,
Ross

[Ross Black]

> assumes that they are 32bit values.

sorry, I meant to say 16bit.

I discovered that our production machines specify /proc/sys/kernel/pid_max = 4000000  (hence the large pid / thread ids)

Looking through the VanillaChronicle code, I can see that the value written to the index always uses 16bits for the thread id and 48 bits for the data offset.
Could this perhaps be configurable?

[Peter Lawrey]

The code can be adapted to 24-bit instead of 16-bit process/thread ids.

It would be useful to have actual range of possible ids instead of guessing. :|

Or rethink how the thread ids are handled.

--
You received this message because you are subscribed to the Google Groups "Java Chronicle" group.
To unsubscribe from this group and stop receiving emails from it, send an email to java-chronicl...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Ross Black]

Hi Peter,

I am happy to submit a patch to allow 24 bit thread ids, if that is the path you would like.
Would you prefer this to be driven by configuration in VanillaChronicleConfig or just hardcoded at 24bits?

As an alternative (if you are rethinking thread-ids) I would suggest that VanillaChronicleAppender instances not be related to a thread at all.
When using VanillaChronicle I initially found it confusing that calling createAppender did not actually create a new instance each time it was called, depending on the thread calling it.

The pattern I would prefer is for createAppender to actually create a new appender instance each time it is called, regardless of the caller thread.  It is then the client responsibility to ensure that the appender is only used from a single thread (the appender could implement some checking if desired).  If this pattern is used, VanillaChronicle can then keep a simple counter that is then used to assign each Appender instance a unique value.  It would also eliminate all of the ThreadLocal lookup code, which makes it more difficult to ensure that resources are closed properly.
Just my 2c.  :-)

Thanks,
Ross

[Peter Lawrey]

There is code which assumes only 16-bits of the threadId is used, which is the reason for the check.

To fix this, you would need to change the format of the index from a 64-bit value containing a 16-bit threadId and a 48-bit offset to be an index which contains a 24-bit thread id and a 40-bit offset.

[Ross Black]

Hi Peter,

I have created pull requests for 2 different options:

https://github.com/OpenHFT/Java-Chronicle/pull/64 - Allow the thread id to be 24 bits.
This is the simplest change.  I extracted the size of the thread id etc into some constants in VanillaChronicleConfig.


https://github.com/OpenHFT/Java-Chronicle/pull/65 - Proposal to remove thread id entirely
This changeset removes thread-id and replaces it with an id that is sequentially allocated from VanillaChronicle.
It also removes all of the ThreadLocal and cache lookup, essentially pushing this responsibility to the client (if required).

Ross