Closed Bug 1591132 Opened 11 months ago Closed 11 months ago

Make it easier to enable and disable hashtable assertions.

Categories

(Core :: XPCOM, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

I've been poking at the places code, and there is scary code like https://searchfox.org/mozilla-central/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/toolkit/components/places/History.cpp#2170 and such that I'm refactoring.

I'm moving them to use more modern hash tables and iterators, and it'd be nice if we could see if this bug still exists and if so what's the root cause more easily (like enabling it when DIAGNOSTIC_ASSERTs are enabled).

I want to land a few patches to make this easier, for now. And will measure the perf impact of enabling them on all hashtables and find a way to opt-into them otherwise.

I want to maybe enable some of these checks in DIAGNOSTIC_ASSERT builds.

The whole type is compiled out in release builds at the moment.

Default constructors of members run if not specified there, and these ifdefs
are ugly.

Put them behind a MOZ_HASH_TABLE_CHECKS_ENABLED define, which right now is only
defined in DEBUG builds, preserving behavior.

MakeImmutable becomes an empty inline function when disabled, which should be
zero-cost.

I'm pretty sure they can be relaxed, as they don't guard any other memory, and
this should reduce their cost.

Also remove useless mutable keywords while at it. The checker is marked as
mutable itself.

It's a bool, should use a bool.

Blocks: 1591149
Attachment #9103988 - Attachment is obsolete: true
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/761eaa4c4f63
Make hashtable checker use MOZ_RELEASE_ASSERT. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/c6cd0b11f18d
Remove some useless ifdefs. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/25ded18e0a55
Make it easy to switch on and off these assertions in different build configurations. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/7a0d7c5c5fad
Use an atomic bool for Checker::mIsWritable. r=froydnj
You need to log in before you can comment on or make changes to this bug.