search

FutureAIGuru / BrainSimII

Neural Simulator for AGI research and development
http://brainsim.org
MIT License
87 stars 25 forks source link

Prevent Brush from being repeatedly allocated #197

Closed Synchronicity89 closed 11 months ago

FutureAIGuru commented 2 years ago

Hi Kevin—

The color of neurons and synapses is a rainbow based on the value as in NeuronArrayView Line 490. If you want to call, that might be more efficient.

Charles J. Simon, BSEE MSCs Founder [Text Description automatically generated with medium confidence][Logo Description automatically generated][Text, application Description automatically generated] 455 Massachusetts Avenue, Suite 120 Washington, D.C. 20001 425-765-8162 https://FutureAI.guruhttps://futureai.guru/

WASHINGTON, D.C. – SPOKANE, WA.

This e-mail is for the sole use of the intended recipient(s). It contains information that may be confidential and/or legally privileged. If you believe that it has been sent in error, please notify the sender immediately by reply e-mail and destroy all copies of this message. Any disclosure, copying, distribution, or use of this information by anyone other than the intended recipient is prohibited.

From: Synchronicity89 @.> Sent: Friday, February 4, 2022 12:14 PM To: FutureAIGuru/BrainSimII @.> Cc: Subscribed @.***> Subject: Re: [FutureAIGuru/BrainSimII] Prevent Brush from being repeatedly allocated (PR #197)

@Synchronicity89 commented on this pull request.

I am changing this back to draft PR to address these issues with it:

  1. User might be able to change the color of neurons and synapses down the road, so want to avoid hardcoding of colors like Blue, Red
  2. The colors could be added to XML where possible and only allocated once a Module is instantiated.

— Reply to this email directly, view it on GitHubhttps://github.com/FutureAIGuru/BrainSimII/pull/197#pullrequestreview-873534577, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ALP7S7WV5FI2GP4J52I6CNTUZQXOZANCNFSM5NQRX7XQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you are subscribed to this thread.Message ID: @.**@.>>

Synchronicity89 commented 2 years ago

Ah, I will look at NeuronView and its function GetNeuronColor. I'll get metrics on any performance improvement I get. I'll also consider users may ask for some kind of flexibility as to colors (for example for color blind people), as well as allocating brushes only as needed.

Synchronicity89 commented 2 years ago

Updated the commit with latest changes, but have not got metrics on performance yet to prove that it helps. I am getting big lags on a system with 32 GB RAM and an i7. Need to make sure that my changes aren't causing them. Will test on a i9 system with 64GB RAM.

FutureAIGuru commented 2 years ago

Thanks for the update. UI performance should be fine unless the network is huge. We use 16M 4-core ryzens and performance is fine. That doesn’t mean that your code is the issue. The last time I fetched your stuff, it ran fine too.

From: Synchronicity89 @.> Sent: Thursday, February 10, 2022 9:29 AM To: FutureAIGuru/BrainSimII @.> Cc: Charles @.>; Comment @.> Subject: Re: [FutureAIGuru/BrainSimII] Prevent Brush from being repeatedly allocated (PR #197)

Updated the commit with latest changes, but have not got metrics on performance yet to prove that it helps. I am getting big lags on a system with 32 GB RAM and an i7. Need to make sure that my changes aren't causing them. Will test on a i9 system with 64GB RAM.

— Reply to this email directly, view it on GitHubhttps://github.com/FutureAIGuru/BrainSimII/pull/197#issuecomment-1035205509, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ALP7S7UJ7APEY4IFL46R2ADU2PYUJANCNFSM5NQRX7XQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you commented.Message ID: @.**@.>>

Synchronicity89 commented 2 years ago

This only a performance refactor. No change in functionality.

I tested the changes on a i9 with 64GB, and it ran fine, with colors looking the same as before. I didn't record exact metrics but the performance was about the same, though theoretically it probably is initially slower to use a cache for brushes rather than allocate them all, because allocation in C# is very fast. This is somewhat mitigated by preloading the cache with a good number of commonly used colors.

Any performance advantage will probably come from completely avoiding GC memory cleanup of brushes. The brushes won't be cleaned up because the cache will be holding on to them. If every possible color of brush is cached that might mean millions of brushes in the cache, but that could be optimized later to reduce the possible number of colors. So far I haven't noticed the number of cached colors go past 100.

Changes made for the sake of performance should usually be left until the end of development, unless there is a specific need for it. However, I chose something easy for my first PR ever on GitHub. I will probably do a feature if I work on it some more.

NickFutureAI commented 2 years ago

@Synchronicity89

Thank you for this work but in its current state, I cannot approve this request until the below-mentioned "Bugs Created" are corrected. Please let me know if you need clarification or disagree with any of the below.

Features: Color Cache

Functionally equivalent besides:

Bugs Created:

Bugs Corrected:

Synchronicity89 commented 2 years ago

Converted it back to draft. I got an exception when I accidently dragged the mouse. Freezing graphics elements is an interesting idea and it has already been used once in this project to Freeze an image. Introducing Freezing for other graphics objects is probably an architectural decision, and would have to be made robust. One way to implement it might be to create methods like this SolidColorBrush SetOpacity(SolidColorBrush brush, double opacity) { .... }

This would return a brush that was either the original brush, or if it was frozen, a clone, with the new opacity.

It would be nice if some brushes with different opacities were also cached. The cache would have to be redesigned to do so.

I will experiment in my fork with a less risky change. I will try XAML markup to introduce Freezing and see what that does.

BTW: My next PR might be this feature: Importing ONNX model data and generating synaptic weights and neuron charges from that. I have made a POC that shows I can access the raw ONNX data and generate a network from it (thus far in an arbitrary way). So far I have generated this attached XML. FirstMostlyGeneratedFromONNX.zip

The ONNX import POC will be soon be available in the stressTest branch of my fork. It will stress test the Brush Cache by having thousands of colors based off of ONNX model data. My vision/mission with ONNX import would be to bring in ONNX models into BrainSimII and see them in action.

Synchronicity89 commented 2 years ago

I am finished working on this PR except for fixing any bugs or implementing requested changes.

It does what I wanted it to do. It caches frozen brushes of each opacity and color used. It also Freezes all Freezable to speed up graphics. I realize that is an architectural type of change, so it should be used only if that architectural change is opted for.

If nothing else its a proof of concept that caching and freezing Freezable objects appears to work and frees up memory. If I run into a crash due to an invalid operation on a frozen object I'll mark this PR draft again. But so far it runs great.

What I may work on next: In my stressTest branch I might continue to work on importing ONNX files, possibly using Microsoft's Microsoft.ML.OnnxRuntime nuget package to retrain the ONNX-imported spiking neural network. As of this moment, in my fork's stressTest branch the imported ONNX weights are used in an arbitrary manner just to prove they can be retrieved from an onnx file. So it is early days for this attempt.

Caveat: Some gitignore changes in this PR may have to be undone or left out.

NickFutureAI commented 2 years ago

This looks great, all requested fixes were made. I am passing this off to Charles @FutureAIGuru for final approval.