Factor out "not disposable" assertion

[zepumph]

There are 500 usages of this assertion in our project

assert && assert( false, 'dispose is not supported, exists for the lifetime of the sim' );

We now have a supertype that could write this for us. What about supporting an option to Disposable called isDisposable: boolean. @samreid and I were excited about the potential for factoring this nice assertion out for reuse. We ran into this question over in https://github.com/phetsims/phet-io/issues/1810

@pixelzoom, how would you feel about me moving this into common code?

[pixelzoom]

Fine with me.

[pixelzoom]

You'll also need to update the "Memory Management" section of many implementation-notes.md files, for example https://github.com/phetsims/geometric-optics/blob/master/doc/implementation-notes.md#memory-management.

[samreid]

I observed that My Solar System has 48031 Disposable instances. However, when I added that attribute in Disposable, I saw the memory did not change appreciably:

This makes no sense to me since 48031 booleans * 4 bytes per boolean should be measurable.

[samreid]

Also note that some of the 518 occurrences of that string do not extend Disposable.

[zepumph]

@samreid do you agree with the recommendation to use Disposable as a way to factor this out? If something doesn't extend Disposable, we could still use a factored out usage of the assertion like:

  /**
   * @public
   */
  dispose() {
    // assert && assert( false, 'dispose is not supported, exists for the lifetime of the sim' );
    Disposable.assertNotDisposable();
  }

But most cases use the option to Disposable.

[zepumph]

Also, if memory is an issue, we can hide this behind assert so that we don't store that boolean in production. I would be surprised if we ever noticed adding a single boolean to the base class though.

[samreid]

Can you help me understand my arithmetic mistake?

This makes no sense to me since 48031 booleans * 4 bytes per boolean should be measurable.

It should be 192kb = 0.2MB, right?

[pixelzoom]

@samreid said:

... 4 bytes per boolean

I'm not sure who to believe, but that's debatable. For example...

From https://shevchenkonik.com/blog/memory-size-of-boolean:

A boolean is actually 1 byte. But alignment may cause 4 bytes to be used on a 32-bit platform or 8 bytes on a 64-bit platform.

From https://learn.microsoft.com/en-us/office/vba/language/reference/user-interface-help/boolean-data-type:

Boolean variables are stored as 16-bit (2-byte) numbers,

[zepumph]

I'm not saying your arithmetic is wrong, I'm saying that it will always be in the noise, and too hard to see that change given how the browser optimizes or implementing a memory heap.

[samreid]

OK factoring out sounds good to me

[zepumph]

@zepumph did you intend to assign this to someone else? Or are we going to leave this issue in limbo, unreviewed?

It is marked for developer meeting discussion, written into the document, as stated in #436 (comment). I was going to take next steps from there.

(1) By searching for "dispose is not supported, exists for the lifetime of the sim", many cases have been missed. I identified an additional 37 cases (addressed in above commits) by searching for "assert && assert( false". I did not inspect the additional 142 cases in code that I'm not responsible for.

That is very helpful. I couldn't think of what else to search for and was going to ask about it during tomorrow's discussion. I will take a look at the other cases in non pixelzoom sims.

(2) In many places, Disposable.assertNotDisposable has been used where isDisposable: false should be used. I'm planning to fix this for my sims.

This is stated in my previous comment about how I didn't think it was worth converting old usages. This is more a pattern that sets us up well for the future. I was not interested in (nor did I think PhET was interested in paying for) me to convert all the Disposable descendants to the better option pattern.

(3) implementation-notes.md that describes this pattern is now incorrect/incomplete. The notes describe overriding dispose, but say nothing about use of isDisposable: false . I'm planning to fix this for my sims.

Sounds good to me! Thanks.

[pixelzoom]

All the sims that I'm responsible for are now using isDisposable: false where appropriate, and implementation-notes.md has been updated.

[zepumph]

PSA complete. I'll take a look at the other usages of assert && assert( false . . . before closing

[zepumph]

Searching for this assert && assert\( false.*(dispose|exists), there were only 9 usages to worry about.