Clean up LSClockState

[ryanrhee]

• pull out methods that don't need self into static functions
• use NSFastEnumeration instead of C for-loops
• wrap the update call in init to run in _sampleQueue

[ryanrhee]

CI seems to have failed because the submodule update failed:

$ git submodule update --init --recursive
Submodule 'lightstep-tracer-common' (git@github.com:lightstep/lightstep-tracer-common.git) registered for path 'lightstep-tracer-common'
Cloning into '/Users/travis/build/lightstep/lightstep-tracer-objc/lightstep-tracer-common'...
Warning: Permanently added 'github.com,192.30.253.112' (RSA) to the list of known hosts.
Permission denied (publickey).
fatal: Could not read from remote repository.

I imagine CI isn't configured correctly.

[bcronin]

The changes in the code looks correct and fine, however admittedly it's a little unclear to me what the functional difference is.

The loop style changes seem to be purely code style clean-up (which is great, but just want to confirm there's no functional change).

Moving _persistSamplesToUserDefaults to a static function also seems to be functionally equivalent to the prior code since the immediate caller references self

The final change is to move the init call to _update into a dispatch_sync call. I don't fully understand the desired outcome of this change. Based on my understanding, it means the _update will happen serially since it's in a queue now and it may (?) happen in a different thread but not necessarily ("As an optimization, this function invokes the block on the current thread when possible."). Since this is in the init function, it seems like there's nothing that could "skip in line" so it's not clear to me what the effect of putting it in the _samplesQueue is versus running it directly.

Aside: the changes in your PR aside, do you see any concerns with thread-safety in this code? I'm a rusty on the details of GCD, but if the _sampleQueue functions are potentially run in a different thread than the any access to samples (which is accessible outside the class) then it seems like there could be concurrent access to an NSMutableArray. If that's the case, then it seems possible that the input to persistSamplesToUserDefaults could be corrupted.

Apologies for the many questions here, but as mentioned, my Obj-C is rusty and I'm struggling a bit to pinpoint the desired changes in the code execution beyond the code style simplification.

[ryanrhee]

The changes in the code looks correct and fine, however admittedly it's a little unclear to me what the functional difference is.

With the exception of wrapping one of the functions in the initializer inside a dispatch_sync, all other changes are mostly cosmetic in nature. (The for-loop style change will likely have a small perf improvement, but that's not the reason for the change.)

The final change is to move the init call to _update into a dispatch_sync call. I don't fully understand the desired outcome of this change. Based on my understanding, it means the _update will happen serially since it's in a queue now and it may (?) happen in a different thread but not necessarily ("As an optimization, this function invokes the block on the current thread when possible."). Since this is in the init function, it seems like there's nothing that could "skip in line" so it's not clear to me what the effect of putting it in the _samplesQueue is versus running it directly.

It's to make sure all access to _samples happens on the _samplesQueue thread.

Aside: the changes in your PR aside, do you see any concerns with thread-safety in this code? I'm a rusty on the details of GCD, but if the _sampleQueue functions are potentially run in a different thread than the any access to samples (which is accessible outside the class) then it seems like there could be concurrent access to an NSMutableArray. If that's the case, then it seems possible that the input to persistSamplesToUserDefaults could be corrupted.

AFAIK, all outwards functions that access _samples or self.samples punts out to _samplesQueue, so there aren't any obvious thread safety concerns. What I mean is, since the queue is marked as DISPATCH_QUEUE_SERIAL, regardless of which thread its blocks are run on, only one will execute at a time.

Let me know if you have any more questions.

[bcronin]

(FYI, I'm trying to get the TravisCI issue resolved)

[ryanrhee]

CI is passing.

[ryanrhee]

@bcronin FYI i can't merge this since I don't have write access.