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

[Pip Cet]

On Wed, Jul 10, 2019 at 3:19 AM Daniel Colascione <dancol@dancol.org> wrote:
> Thanks for debugging this problem. It really is nasty. AIUI, the problem
> is that hash-consing the hash vectors might unify vectors that happen to
> have the same contents under one hashing scheme but different contents
> under a different hashing scheme, so when we rehash lazily, we correctly
> rehash one hash table and corrupt a different hash table's index array by
> side effect. There are two proposed solutions:
>
> 1) Copy the hash table internal vectors on rehash, and
> 2) "Freeze" hash tables by eliminating the index arrays and rebuilding
> them all eagerly on dump start.

That summary is correct, I think.

> #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, slightly slower,
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).

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).

> #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.

> 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.