Faster support of choicesAllMatching #10273
Comments
casella
added
COMP/OMC/Frontend
|
Adding @rfranke to the loop, he reported the specific test case. |
- Only expand packages when doing lookup for getAllSubtypeOf, since fully instantiating them is unnecessary and slow in that context. Fixes OpenModelica#10273
|
#10275 improves the performance significantly by trying to avoid fully instantiating packages during lookup when it's not needed for I think it could be improved further since the implementation right now only uses the NF for doing lookup, so it's doing a lot of unnecessary things. But that would require completely rewriting the whole implementation, and it's probably good enough for now. |
- Only expand packages when doing lookup for getAllSubtypeOf, since fully instantiating them is unnecessary and slow in that context. Fixes #10273
|
@rfranke please test and report. |
|
Wow. It's like day and night on my Linux development machine. Looking forward to trying out the new nightly build on my Windows laptop tomorrow! |
|
One thing changed to the worse: most Fluid models define a Medium locally to the model, e.g. model PumpingSystem "Model of a pumping system for drinking water"
extends Modelica.Icons.Example;
replaceable package Medium = Modelica.Media.Water.StandardWaterOnePhase constrainedby Modelica.Media.Interfaces.PartialMedium;
Modelica.Fluid.Sources.FixedBoundary source(redeclare package Medium = Medium);
...
end PumpingSystem;OMEdit doesn't show this in the list of Medium options anymore. It only appears if it had already been selected before (e.g. in an existing example). It does not appear in newly added component models. Previous versions were showing model ModifiedPumpingSystem
extends PumpingSystem(redeclare replaceable package Medium = SomethingOther);
end ModifiedPumpingSystem;A component using |
|
@rfranke: It looks like the issue is that OMEdit calls: getAllSubtypeOf(Modelica.Media.Interfaces.PartialMedium,Modelica.Fluid.Sources.BaseClasses.PartialSource,false,false,false)In #10277 I changed However, OMEdit gives If |
|
#10487 improves I also removed a weirdly placed |
|
@rfranke please try this with the next nightly. If that is OK for you, we can cherry pick this fix into 1.21.0 |
|
Thank you for the change of It works better now, but still a bit confusing. It works when adding e.g. a @adeas31 would need to also change the call to |
|
#10506 will fix this for old api which is still default in OMEdit. Need more work to fix this for instance api. |
|
@adeas31 can you please also cherry-pick this on maintenance/v1.21? |
|
Looks good! |
|
Unfortunately testsuite-clang-metamodelica failed that should be unrelated though :( |
|
Try to force push to the branch so that license cla will be resend. |
|
Huuh. Removed the additional commit again and now both passed: tests and cla. |
|
Great, thanks @rfranke! |
|
#10536 makes sure that this works as expected when instance API is used. |
Install the ThermofluidStream library, open the
ThermofluidStream.Processes.Tests.ConvectivePipeand double click on thethermalConvectioncomponent. Getting the parameter input window requires about one minute.Checking the omeditcommunication.log file shows the culprit of such slow performance:
The bulk of the time is spent getting all the subtypes of ThermofluidStream.Media.myMedia.Interfaces.PartialMedium.
The way this is currently implemented is not efficient. According to @adrpo, it should be replaced by caching this information during parsing. Then, this should be added to the new instantiation-based interface, which currently doesn't support
choicesAllMatching.@adrpo, @perost, can you agree on a plan so that @perost can implement it at his earliest convenience?
The text was updated successfully, but these errors were encountered: