shiny_attributes performance improvments

[jeanetteclark]

hey @cboettig @amoeba or anyone else that is interested - I've been thinking about the EML shiny_attributes function. We typically use it a lot and its generally really helpful, but recently it has become unbelievably slow to the point of being unusable. I think I've tracked at least part of the problem down to this call units::valid_udunits_prefixes(), (line 190 of get_unit_id.R), though I'm sure there are other places that could be faster. I'm not sure what caused the app to suddenly be so slow but my guess is some underlying change to theunits library. I'm wondering what you all think of the best path forward to make this workable again.

In no particular order some options I thought of are:

• deprecating the guess unit support, since it weighs down the package quite a lot
  • Would this impact downstream packages like EMLassemblyline? @clnsmth @earnaud
• adding the guess unit support as an argument to shiny_attributes, so it can be turned off if needed
• reworking the get_unit_id architecture so that it only calls that function once
  • Not sure if I personally can sink the time I expect would be required to do this into the feature

I think there has also been some talk of taking the shiny_attributes function out of this package entirely - but that was some years ago - not sure where opinions lie there.

I welcome any thoughts, my main goal is to get this working again in some form so our team can use it.

[amoeba]

Thanks for the report, @jeanetteclark. We should definitely invest some time getting this working again. Your idea about line 190 being a culprit seems pretty reasonable after a quick glance. It does seem to do work that is identical on every invocation.

It would probably help me to get an MRE for this so I can poke around. I'll try that now and get back to you. Does this happen with attribute lists of any size or just larger ones? I'd also like to get at the bottom of why this is happening now and not before.

[jeanetteclark]

@amoeba it actually happens for me even if I don't pass any arguments and it just spins up an empty table. Typically, it never loads, or it takes several minutes, which leads me to believe I'm probably missing something. I got to that particular line by running profvis on get_unit_id

[amoeba]

Typically, it never loads, or it takes several minutes

Very interesting. It is slow to load but I get it to load in about 8s on both my local machine and datateam. That's still too slow for my test but I'm curious why it's so much slower for you. Can you dump a devtools::session_info() in here?

[jeanetteclark]

After cleaning up my session a bit I am also down to 8s or so to load a blank table, and somewhat reasonable (but still a little slow) times for tables with moderate number of attributes

[amoeba]

@jeanetteclark sent me a devtools::session_info() out of band of this conversation though it didn't give me any pointers.

I was able to reproduce the performance @jeanetteclark was seeing and saw the same culprit. I opted to use an Environment to cache the units::valid_udunits_prefixes call and test that the startup time and editing slowness went away.

See #333. @jeanetteclark would you mind giving that a whirl just to confirm it works equally well for you?

[jeanetteclark]

Cool @amoeba! I thought about caching, but wouldn't have thought to use a global var for it. I just tested and it works much better. I can have a look at those tests and see if I can fix them this afternoon

[amoeba]

Sounds good, thanks @jeanetteclark.

[amoeba]

Closing since this change was merged and we're cutting a release.