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]

@samreid and I had a co-review in the middle of the above commits, and he helped me clean up the implementation a lot! Thanks. Now we have Disposable.initializeDisposable() which is used in conjunction with PhetioObject.initializePhetioObject. All of that is to support Node.mutate, but that baggage is well worth it for the mutate gains we have. Work done:

I converted all of these assertions to use Disposable.assertNotDisposable()

1. Mass rename all usages of the assertion to the new function
2. find/replace this regex to get some imports in there automatically (got 80%)

\n\n(import \w+ from '((\.\.\/)*))
\n\nimport Disposable from '$2axon/js/Disposable.js';\n$1

3. Next cover cases like import { NodeOptions } from ...

\n\n(import .+ from '((\.\.\/)*))
\n\nimport Disposable from '$2axon/js/Disposable.js';\n$1

4. Fix 23 errors where the first import wasn't to another repo, but internal (needed more ../
5. Fix 4 errors where '/axon' existed because above we should have selected on + instead of a * amount of ../

Tested and committed!

[zepumph]

Alright. I removed some usages from Joist above, and converted all of acid-base-solutions as an exemplar and to test the code changes. Next is a PSA about this at dev meeting.

PSA: Please note that disposable now supports your "class is not disposable" case!

Work done in this issue:

1. If your class extends Disposable, use isDisposable: false for automatic support of this and no need to override dispose()
2. Disposable.assertNotDisposable() should be called in you dispose() method if (1) isn't applicable. Do not use a custom assertion.
3. A lint rule keeps people from saying dispose is not supported, exists for the lifetime of the sim, but that won't catch all cases.

I do not plan to convert other cases to use the isDisposable flag, but I think that is a better option generally, and we should be using that especially for Nodes which already support an option pattern very well.

Examples:

AboutDialog is not allowed to be disposed:
https://github.com/phetsims/joist/blob/a801b7c1b8c0bd125f6cc57957ca448a7b2affb7/js/AboutDialog.ts#L62

MySolutionModel does not extend Disposable, and it is a bit overkill to make it just for this, so use the factored out static method:
https://github.com/phetsims/acid-base-solutions/blob/07f6de02532bcb486bb3e944230b3af8b8f26cbd/js/mysolution/model/MySolutionModel.ts#L115-L119

[pixelzoom]

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

[pixelzoom]

Three problem with the changes made in sims:

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

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

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

[pixelzoom]

@zepumph said:

I removed some usages from Joist above, and converted all of acid-base-solutions as an exemplar and to test the code changes. Next is a PSA about this at dev meeting.

acid-base-solutions is not much of an exemplar. It's currently using Disposable.assertNotDisposable where isDisposable: false is preferred.

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