606236 - MNHT calls WB on non-pointer values and isn't consistent

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Description

[Tommy Reilly]

addVersionedBindings(bindings, name, compat_nss, AvmCore::makeMGSBinding(methodCount, BKIND_METHOD));

we want to make the write barrier stricter but this has to be addressed first.

MNHT.h sez:

        BKIND_METHOD            = 1,        // MethodEnv*                           001

Which is a lie in this case because methodCount ain't a MethodEnv*

Comment 1

[Steven Johnson]

MNHT has lots of client code who play fast and loose with what they hand it; some cleanup would probably be a very good idea

Comment 2

[Edwin Smith]

(In reply to comment #1)
> MNHT has lots of client code who play fast and loose with what they hand it;
> some cleanup would probably be a very good idea
                     ^^^^^^^^ definitely

This is one of the reasons we are held hostage to Atom; lets fix abuses aggressively as we find them.  Or, at least, catalog them in a tracking bug.

Comment 3

[Edwin Smith]

This might be a dupe of a bug Lars ran into with exact-marking.  I remember talking to him about it and I'm not sure if he wrote it up as a blocker or just found a workaround.  Maybe check the exact-marking dependencies.

Comment 4

[Tommy Reilly]

I see comments about this in Lar's exact tracing patch but couldn't find a bug reference or one linked to the exact tracing master bug, I suspect we don't feel strongly about this to make it a blocker on exact tracing?

Comment 5

[Tommy Reilly]

FYI, Lar's rolled a ConserativeTracePointer function to handle this and other cases of malfeasance

Comment 6

[Lars T Hansen]

Malfeasance is right...

Comment 7

[Steven Johnson]

IMHO the right long-term fix is to upgrade MNHT to be type-safe with various payloads, a la https://bugzilla.mozilla.org/show_bug.cgi?id=568664 -- not a trivial change, alas

Comment 8

[Tommy Reilly]

Attachment: Not the right not term fix but lets my strict write barrier assertions fly

Doesn't help exact tracing but it at least delineates the malfeasance which isn't that bad actually.

Comment 9

[Steven Johnson]

Comment on attachment 485415 [details] [diff] [review]
Not the right not term fix but lets my strict write barrier assertions fly

yuck, holding nose but arguably better than nothing. comments:

-- instead of a new arg to put(), maybe have put_gc() and put_nongc(), since put() is typically on a hot path and you almost always know at compile time which you want

-- gonna need a lot more specialization that Traits, I suspect. Also worth doing a scan of Flash/AIR glue code to find people using this.

-- extra credit: make two thin-inline templated wrappers on top of MNHT, one for GC, one for non-GC, thus avoiding the overloading of add()?

Comment 10

[Tommy Reilly]

Attachment: getting warmer?

Comment 11

[Steven Johnson]

Comment on attachment 490941 [details] [diff] [review]
getting warmer?

Oh yeah. Just did a quick skim, not full review, but this looks much happier.

Comment 12

[Tommy Reilly]

Pre-review Checklist:

sandbox
perf
code size

Anything else?

Comment 13

[Tommy Reilly]

Attachment: templatized MultinameHashtable

sandbox passes
code size is a wash on gcc and msvc
perf is a wash
no hits on MultinameHashtable in FR

Comment 14

[Tommy Reilly]

I didn't wait for the perf run to finish, there was one repeatable win:

                                      avm          avm2
test                          best    avg   best    avg  %dBst  %dAvg
Metric: time 
Dir: jsbench/typed/
  LUFact                      2530 2532.6   2227 2230.8   12.0   11.9 ++

Comment 15

[Steven Johnson]

Comment on attachment 491504 [details] [diff] [review]
templatized MultinameHashtable

One real suggestion: this code pattern, seen in a few places:

        int bitMask = numQuads - 1;
        // Note: Mask off MSB to avoid negative indices.  Mask off bottom
        // 3 bits because it doesn't contribute to hash.  Quad it
        // because names, namespaces, and values are stored adjacently.
        unsigned i = ((0x7FFFFFF8 & (uintptr_t)mnameName)>>3) & bitMask;

has always seemed wonky to be and probably time to consider fixing it: 

(1) "Mask off MSB to avoid negative indices", but uintptr_t is unsigned, so this is pointless
(2) "Mask off bottom 3 bits" is a waste since we are shifting those bits out anyway, via >>3
(3) "Quad it because names, namespaces, and values are stored adjacently" is nonsensical and I think applies to a long-dead implementation

Can anyone explain why this is just as good:

        uintptr_t bitMask = numQuads - 1;
        uintptr_t i = (uintptr_t(mnameName)>>3) & bitMask;

nits:

-- tabbing in Domain.h, MultinameHashtable.h, PoolObject.h, Traits.h (vertical alignment of members)

-- // point to GC memory, highly unlikely and its worth keeping us
                                              ^^^ it's

-- // To be specific   how to declare getNSSet's return value elluded
                    ^,                                        ^^^^^^^ eluded

idle thoughts:

-- find() and rehash() use "int" and "unsigned"; I wonder if now is a good time to convert them to C99 types?

-- I wonder if isFull() should be inlined?

Comment 16

[Tommy Reilly]

I'll fix the comments and code alignment but not the other changes, I'm just trying to unblock my stricter write barriers which Lar's wanted checked in yesterday.

Comment 17

[Edwin Smith]

Comment on attachment 491504 [details] [diff] [review]
templatized MultinameHashtable

Domain.h:
  - fix alignment of m_namedTraits and m_namedScriptsMap.
    (really, I shouldn't have to ask!  that the messy formatting got this
    far wastes my time to write about it and your time to fix it.  just do it 
    up front, please).

The new template functions in MultinameHashtable-impl.h are not marked inline.  Should they be?  If not, explicit instantiation might be required, but that's okay, we control the use sites.  IIRC, explicit instantiations can go in MNHT.cpp, like this:  (or elsewhere, for that matter).

   template class MultinameHashtable<Binding, BindingType>;
   template class MultinameHashtable<Traits*, GCObjectType>;

I think there will be enough movement to re-review after this, but not re-SR.

Comment 18

[Tommy Reilly]

(In reply to comment #17)
> Comment on attachment 491504 [details] [diff] [review]
> templatized MultinameHashtable
> 
> Domain.h:
>   - fix alignment of m_namedTraits and m_namedScriptsMap.
>     (really, I shouldn't have to ask!  that the messy formatting got this
>     far wastes my time to write about it and your time to fix it.  just do it 
>     up front, please).

Will do, I've never paid attention to the alignment of the field names before, we don't do it everywhere (we don't do it at all in MMgc) and I had to look at it a couple times to figure out what Steven was talking about.

Comment 19

[Tommy Reilly]

(In reply to comment #17)

> The new template functions in MultinameHashtable-impl.h are not marked inline. 
> Should they be? 

This bug isn't about revisiting whether or not certain methods should be inlined.  These are out of line as they were before.

> If not, explicit instantiation might be required, but that's
> okay, we control the use sites.  IIRC, explicit instantiations can go in
> MNHT.cpp, like this:  (or elsewhere, for that matter).
> 
>    template class MultinameHashtable<Binding, BindingType>;
>    template class MultinameHashtable<Traits*, GCObjectType>;
> 
> I think there will be enough movement to re-review after this, but not re-SR.

This is exactly what the patch does.   See MultinameHashtable.cpp

Comment 20

[Tommy Reilly]

I think isFull(In reply to comment #15)
> -- I wonder if isFull() should be inlined?

isFull, Init and perhaps the dtor and getMulti could perhaps be inlined, I'll open a separate bug.

Comment 21

[Tommy Reilly]

https://bugzilla.mozilla.org/show_bug.cgi?id=613490

Comment 22

[Tommy Reilly]

https://bugzilla.mozilla.org/show_bug.cgi?id=613491

Comment 23

[Tamarin Bot]

changeset: 5566:a67f06077185
user:      Tommy Reilly <[e-mail]>
summary:   Refactor MultinameHashtable as a template class to get proper write barrier usage (r=stejohns,sr=edwsmith,bug=606236)

http://hg.mozilla.org/tamarin-redux/rev/a67f06077185

Comment 24

[Tommy Reilly]

(In reply to comment #19)

> > I think there will be enough movement to re-review after this, but not re-SR.

There was no movement since the suggestion was to do something the patch already did so I pushed it.

Comment 25

[Tommy Reilly]

Also logged this bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=613196

Comment 26

[Edwin Smith]

(In reply to comment #19)

> This is exactly what the patch does.   See MultinameHashtable.cpp

Fair enough -- but then why are all those functions in an -impl.h file?  They could go in MultinameHashtable.cpp.

Comment 27

[Tommy Reilly]

(In reply to comment #26)
> Fair enough -- but then why are all those functions in an -impl.h file?  They
> could go in MultinameHashtable.cpp.

Good question, I had my stejohns hat on and was just following the avmplusList precedent.

Comment 28

[Steven Johnson]

(In reply to comment #26)
> Fair enough -- but then why are all those functions in an -impl.h file?  They
> could go in MultinameHashtable.cpp.

There is a good reason: this allows downstream users to create additional custom instantiations more easily, without needing to edit our .cpp file: they can just #include the -impl.h file and instantiate.