bug#36447: 27.0.50; New "Unknown keyword" errors

[Daniel Colascione]

> On Wed, Jul 10, 2019 at 3:19 AM Daniel Colascione <dancol@dancol.org>
> wrote:
>> #1 works, but it's somewhat inefficient.
>>
>> #2 is a variant of #1, in a sense. Instead of copying the hash table
>> vectors and mutating them, we rebuild them from scratch. I don't
>> understand why we have to do that eagerly.
>
> I'm sorry if I suggested that we must do that. On the contrary, both
> options are entirely viable, though on balance I prefer the eager
> version.
>
> The lazy approach makes the code more complicated,

I don't think doing it eagerly is a complexity win. The eager patch posted
upthread is a wash.

> slightly slower,

No it doesn't. The hash table branch on negative count should be predicted
correctly and involves a test against a cache line we're loading anyway.
We can add explicit likely() and unlikely() annotations too.

> and introduced what appears to me to be at least one bug
> (Fhash_table_count returns a negative integer if called with a
> non-rehashed hastable).

That's a trivial oversight and not an inherent difficulty in the lazy
approach.

> The eager approach slows down startup by a fraction of a millisecond
> (it might be an entire millisecond if your Emacs session doesn't
> access any of the dumped hash tables at all, but since it does tend to
> do that, it'll be less in practice).

A millisecond here and a millisecond there and things end up costing real
time. A lazy approach isn't any harder than an eager one and

>> #1 isn't as bad as you might think.
>
> But it's not as good as "do #1 but only if PURE_P", which no longer works.
>
>> It's the same work either way, except that if we copy, when we grow the
>> hash table, we can actually free the original vectors.
>
> I'd like to restrict #1 to hash tables which are somehow immutable,
> either because they're pure or because we actually introduce immutable
> objects, so they'd never grow. Mutable hash tables must not share
> their index vectors anyway.

Did you miss the part about avoiding copy-on-write faults? Those hash
table vectors are going to be copied anyway, and we might as well do it
ourselves instead of making the kernel do it. (unexec has the same
inefficiency, BTW.) We should just do #1 for all hash tables.

>> IMHO, the right approach is to check in #1 for now and switch to a
>> #2-like
>> approach once master is stable. Noticing that we don't actually have to
>> store the hash table internal arrays in the dump is a good catch.
>
> I agree, but I think we should discuss the future of pure space, too.
> Maybe we should have a separate bug for that, though.

Sure.