Re: scratch/igc fe90d556834: Make glyphs ambiguous roots

Re: scratch/igc fe90d556834: Make glyphs ambiguous roots

[Helmut Eller]

On Sun, Jul 28 2024, Gerd Moellmann wrote:

>     We cannot trace glyphs in fix_window because we don't have exclusive
>     access to glyph_rows and glyphs. (An optimal solution would allocate
>     glyphs from MPS, and change data structures in a way that everything
>     needed (row->used, row->enabled_p) is available for the scanner.)

I don't understand this.  Glyph_rows aren't allocated in MPS-managed
memory so there are no memory barriers to worry about.  While fix_frame
runs, all other threads are suspended.  So why don't we have exclusive
access?  And why is it not a problem for FRAME_FONT, FRAME_DISPLAY_INFO,
etc?

Re: scratch/igc fe90d556834: Make glyphs ambiguous roots

[Gerd Möllmann]

Helmut Eller <eller.helmut@gmail.com> writes:

> On Sun, Jul 28 2024, Gerd Moellmann wrote:
>
>>     We cannot trace glyphs in fix_window because we don't have exclusive
>>     access to glyph_rows and glyphs. (An optimal solution would allocate
>>     glyphs from MPS, and change data structures in a way that everything
>>     needed (row->used, row->enabled_p) is available for the scanner.)
>
> I don't understand this.  Glyph_rows aren't allocated in MPS-managed
> memory so there are no memory barriers to worry about.

True.

> While fix_frame runs, all other threads are suspended.

I don't think that is guranteed. but please check.

Frames are not roots, so fix_frame can only assume exclusive access to
the frame struct itself. Other threads can work on glyph_rows, glyphs
and so on while fix_frame runs.

> So why don't we have exclusive access? And why is it not a problem for
> FRAME_FONT, FRAME_DISPLAY_INFO, etc?

FRAME_X_DISPLAY_INFO has the same problem. It's just that I had the
glyphs on my todo list. With frame->font I don't see a problem. Fonts
are pvecs so we only have to fix the pointer to it, and fix_font should
do the rest.

BTW, unrelated. Do you observe problems in clear_face_cache? I'm getting
EXC_BAD_ACCESS from accessing face->font->driver, sometimes, as if the
face hadn't been scanned. But the face looks okay, as far as the
face_cache is concerned. Happens since I change the default font to
JetBrains Mono. (And happened before that change above.)

Re: scratch/igc fe90d556834: Make glyphs ambiguous roots

[Helmut Eller]

On Mon, Jul 29 2024, Gerd Möllmann wrote:

>> While fix_frame runs, all other threads are suspended.
>
> I don't think that is guranteed. but please check.
>
> Frames are not roots, so fix_frame can only assume exclusive access to
> the frame struct itself. Other threads can work on glyph_rows, glyphs
> and so on while fix_frame runs.

The doc
https://memory-pool-system.readthedocs.io/en/latest/topic/format.html#cautions
says:

  1. The MPS guarantees that format methods have exclusive access to the
     object for the duration of the call. This guarantee may entail
     suspending arbitrary threads. The methods that manipulate the object
     must not perform any sort of inter-thread locking or communication.
  ...

  6. Subject to the above constraints, format methods can freely access:
     a. memory inside the object or block that they have been asked to
        look at;
     b. MPS-managed memory in pools that do not protect their contents;
     c. memory not managed by the MPS.

While the doc doesn't guarantee that all other (registered) threads are
suspended, it's hard to imagine what else MPS could do to stop other
threads from messing up the object.  (MPS must remove hardware barriers,
so that's not an option.)

>> So why don't we have exclusive access? And why is it not a problem for
>> FRAME_FONT, FRAME_DISPLAY_INFO, etc?
>
> FRAME_X_DISPLAY_INFO has the same problem. It's just that I had the
> glyphs on my todo list. With frame->font I don't see a problem. Fonts
> are pvecs so we only have to fix the pointer to it, and fix_font should
> do the rest.

There is no frame->font.  Only a frame->output_data.ns->font.  And
ns_output is not part of the frame struct.

> BTW, unrelated. Do you observe problems in clear_face_cache?

Nothing that I'm aware off.

Re: scratch/igc fe90d556834: Make glyphs ambiguous roots

[Gerd Möllmann]

Helmut Eller <eller.helmut@gmail.com> writes:

> On Mon, Jul 29 2024, Gerd Möllmann wrote:
>
>>> While fix_frame runs, all other threads are suspended.
>>
>> I don't think that is guranteed. but please check.
>>
>> Frames are not roots, so fix_frame can only assume exclusive access to
>> the frame struct itself. Other threads can work on glyph_rows, glyphs
>> and so on while fix_frame runs.
>
> The doc
> https://memory-pool-system.readthedocs.io/en/latest/topic/format.html#cautions
> says:
>
>   1. The MPS guarantees that format methods have exclusive access to the
>      object for the duration of the call. This guarantee may entail
>      suspending arbitrary threads. The methods that manipulate the object
>      must not perform any sort of inter-thread locking or communication.
>   ...
>
>   6. Subject to the above constraints, format methods can freely access:
>      a. memory inside the object or block that they have been asked to
>         look at;
>      b. MPS-managed memory in pools that do not protect their contents;
>      c. memory not managed by the MPS.
>
> While the doc doesn't guarantee that all other (registered) threads are
> suspended, it's hard to imagine what else MPS could do to stop other
> threads from messing up the object.  (MPS must remove hardware barriers,
> so that's not an option.)

Good point, indeed.

I've reverted that change. Thanks for checking!

>
>>> So why don't we have exclusive access? And why is it not a problem for
>>> FRAME_FONT, FRAME_DISPLAY_INFO, etc?
>>
>> FRAME_X_DISPLAY_INFO has the same problem. It's just that I had the
>> glyphs on my todo list. With frame->font I don't see a problem. Fonts
>> are pvecs so we only have to fix the pointer to it, and fix_font should
>> do the rest.
>
> There is no frame->font.  Only a frame->output_data.ns->font.  And
> ns_output is not part of the frame struct.
>
>> BTW, unrelated. Do you observe problems in clear_face_cache?
>
> Nothing that I'm aware off.

Too bad, then it might be another macOS thing :-(.

Re: scratch/igc fe90d556834: Make glyphs ambiguous roots

[Gerd Möllmann]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

>> The doc
>> https://memory-pool-system.readthedocs.io/en/latest/topic/format.html#cautions
>> says:
>>
>>   1. The MPS guarantees that format methods have exclusive access to the
>>      object for the duration of the call. This guarantee may entail
>>      suspending arbitrary threads. The methods that manipulate the object
>>      must not perform any sort of inter-thread locking or communication.
>>   ...
>>
>>   6. Subject to the above constraints, format methods can freely access:
>>      a. memory inside the object or block that they have been asked to
>>         look at;
>>      b. MPS-managed memory in pools that do not protect their contents;
>>      c. memory not managed by the MPS.
>>
>> While the doc doesn't guarantee that all other (registered) threads are
>> suspended, it's hard to imagine what else MPS could do to stop other
>> threads from messing up the object.  (MPS must remove hardware barriers,
>> so that's not an option.)
>
> Good point, indeed.
>
> I've reverted that change. Thanks for checking!

By the same argument, I think we could simplify face_cache and
image_cache which both use IGC_OBJ_PTR_VEC at the moment. 
IGC_OBJ_PTR_VEC could then go completely.

WDYT?

Re: scratch/igc fe90d556834: Make glyphs ambiguous roots

[Helmut Eller]

On Mon, Jul 29 2024, Gerd Möllmann wrote:

>>> While the doc doesn't guarantee that all other (registered) threads are
>>> suspended, it's hard to imagine what else MPS could do to stop other
>>> threads from messing up the object.  (MPS must remove hardware barriers,
>>> so that's not an option.)
>>
>> Good point, indeed.
>>
>> I've reverted that change. Thanks for checking!
>
> By the same argument, I think we could simplify face_cache and
> image_cache which both use IGC_OBJ_PTR_VEC at the moment. 
> IGC_OBJ_PTR_VEC could then go completely.

You mean, you would use the regular xmalloc instead of igc_make_ptr_vec
to allocate those vectors and trace the vectors in fix_face_cache.  Hm,
yes that's indeed a similar situation.  As long as those caches aren't
roots, I don't see a problem.

I guess this would make the MPS and non-MPS code more alike.  We'd lose
some details for igc-info, though that's a minor issue.

Re: scratch/igc fe90d556834: Make glyphs ambiguous roots

[Gerd Möllmann]

Helmut Eller <eller.helmut@gmail.com> writes:

> On Mon, Jul 29 2024, Gerd Möllmann wrote:
>
>>>> While the doc doesn't guarantee that all other (registered) threads are
>>>> suspended, it's hard to imagine what else MPS could do to stop other
>>>> threads from messing up the object.  (MPS must remove hardware barriers,
>>>> so that's not an option.)
>>>
>>> Good point, indeed.
>>>
>>> I've reverted that change. Thanks for checking!
>>
>> By the same argument, I think we could simplify face_cache and
>> image_cache which both use IGC_OBJ_PTR_VEC at the moment. 
>> IGC_OBJ_PTR_VEC could then go completely.
>
> You mean, you would use the regular xmalloc instead of igc_make_ptr_vec
> to allocate those vectors and trace the vectors in fix_face_cache.  Hm,
> yes that's indeed a similar situation.  As long as those caches aren't
> roots, I don't see a problem.

Yes, axactly. The bucket vectors and face_cache::faces_by_id and
image_cache::images could be xmalloc'd like in the non-MPS case,
and fix_face_cache and fix_image_cache could iterator over them and so
on. 

> I guess this would make the MPS and non-MPS code more alike.  We'd lose
> some details for igc-info, though that's a minor issue.

Yes, and things gets a bit simpler, which I'm personally a big fan of.
I'll put that on my todo list.

Thanks!