Overuse of @decorators.Cache?

[Nat Duca]

Hi folks -

I'm pretty concerned about our use of decorators.Cache. It causes hidden state --- quick inspection of a property and class will look like its creating a class, but then you realize its actually permanently keeping something alive.

For instance, this @cache really tripped me up in its use throughout platform:

http://src.chromium.org/chrome/trunk/src/tools/telemetry/telemetry/core/platform/factory.py

I didn't notice it was persisted [on the module, so basically forever] until I actually debugged it. My eyes just skipped the decorator.

The same sorta strangeness exists in the way platform backends are cached. Reading possible_browser, it looked like we'd make a new backend per possible browser. Except then you dig in and read each file, http://src.chromium.org/chrome/trunk/src/tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py, you realize its actually creating and caching platform_backend.

I'd like to propose this:

• Never use Cached on functions [eg non modules]
• When you're trying to retain results from a calculation, say an average on a big list of numbers, cached seems fine.
• When you're creating a new object that is going to get handed around to other classes and retained, make the lifetimes explicit.

- N

[Tony Gentilcore]

I introduced this a while back because we had dozens of places where
we were using globals to accomplish this in a variety of uniquely
buggy ways. Then we had equally subtle code that might reach in and
play w/ the global. Like:
https://codereview.chromium.org/436963003/diff/1/tools/telemetry/telemetry/core/backends/chrome/android_browser_finder_unittest.py

I'm not tied to it at all, but would like to point at that if we stop
using it, we're going to have to re-implement some form of that global
variable hack in lots of places. If each is a little different, it is
going to be even harder to grok.

What if, instead, we changed the global one to be called @Singleton or
something and make the decorator enforce a naming convention on the
creator. For instance CreateSingletonFoo() -- Where the
"CreateSingleton" is enforced. It could also have some kind of Reset()
helper.

If folks agree on this or a similar pattern, I'd be happy to do a
cleanup sweep to convert everything.

Thoughts?

-Tony

[Dirk Pranke]

I haven't hacked on core telemetry enough to have an informed opinion about your specific proposals, but my experience matches yours exactly: 9 out of 10 times, using a @cache or @memoized decorator (Blink has the latter) makes code much harder to follow.

This also matches the recommendations of the Google Python style guide :).

-- Dirk

On Fri, Aug 1, 2014 at 7:48 PM, 'Nat Duca' via telemetry <tele...@chromium.org> wrote:

[Nat Duca]

Hmm I hadn't thought of some of the use cases you mentioned Tony.

Renaming the right use cases to @Singleton would be great. I buy those totally.

I think the ones I'm wary about are cases where well meaning but hurried folk slap a @cached on a method.

Not urgent to do the change though, I learned something from this thread, thank you all!