PreviewLoader: change 2^(20) to pow(2, 20)?

[Richard Kimberly Heck]

On 3/28/20 7:31 PM, Scott Kostyshak wrote:
> On Sat, Mar 28, 2020 at 02:57:31PM -0400, Richard Kimberly Heck wrote:
>> On 3/28/20 1:10 PM, Scott Kostyshak wrote:
>>> Can someone check whether the attached changes make sense? The first
>>> patch just changes the code to what I think is intended. But is it worth
>>> it to include the <cmath> header for this one expression or should we
>>> put the hardcoded value and in a comment note that it equals 2^20?
>>>
>>> The second patch increases pow(2, 20) to pow(2, 22), which is
>>> PID_MAX_LIMIT on most 64-bit systems, but perhaps there was a reason
>>> pow(2, 20) was used? I don't understand the purpose of these fake PIDs
>>> so I'm not sure.
>> How about (1 << 22) + 1, which we use elsewhere?
> Nice, indeed more elegant. The adapted patches are in at 8f3c95f7 and
> b67ff925.

Yeah, it's a cute trick. I learned it from our code somewhere. But
apparently after I wrote that code! Though someone must have helped me
with that. I don't think I understand that part of the code well enough
to have written it. I have no idea where 2^20 came from.


>> We usually track preview processes by their pids, which we get from
>> ForkedProcess. But since we are waiting for the result in this case, we
>> don't get a real pid (and don't need it), so we just use some arbitrary
>> integer.
> I see. So we just need a unique identifier. It took me a bit to realize
> why we need to be concerned with thread safety but I think I understand
> now: even though each buffer has its own preview loader, and even though
> we are waiting at this point in the code, it might be that two buffers
> are processing previews simultaneously, and the static is shared at the
> class level so it is even shared across buffers.

I think that must be right: Guillaume added the atomic_int bit.

At some point, I audited a lot of the static variables. This one could
probably be made a class member without significant cost, but it's
probably also fine as it is.

Another idea here would be to make InProgressProcesses a map<int,
InProgress> and use *negative* values as 'fake' pids.

Riki