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

[Scott Kostyshak]

On Sat, Mar 28, 2020 at 11:23:27PM -0400, Richard Kimberly Heck wrote:
> 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.

Yeah that makes sense.

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

Ah, that is a nice idea. It would make it easier to debug because it is
easier to tell whether a number is negative than whether it is greater
than 2^22. Another idea would be to have a data member "fake". But I
guess as long as whatever code processes these elements doesn't need to
know if they are fake, it doesn't matter.

Scott
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.lyx.org/pipermail/lyx-devel/attachments/20200329/bcecc81a/attachment.asc>