Page MenuHomePhabricator

Replace uses of LazyBool with LazyBool template
Needs ReviewPublic

Authored by teemperor on Aug 31 2018, 1:51 PM.

Details

Reviewers
davide
Summary

This patch introduces a LazyMember template that allows specifying member variables
which values will be computed once they are read the first time. This is supposed to
replace the previously used LazyBool enum that is partly providing the same functionality
(but you have to do all the checks yourself).

The advantages of LazyMember over handwritten LazyBool code are:

  • No more manual checking when reading the value somewhere.
  • The compiler enforces now that we actually set a value before returning to the user.
  • We can optimize this code further in the future for space/time without

having to revisit all the different places that use LazyBool.

Diff Detail

Event Timeline

teemperor created this revision.Aug 31 2018, 1:51 PM
davide added inline comments.Aug 31 2018, 2:00 PM
include/lldb/DataFormatters/ValueObjectPrinter.h
146

can you typedef?

I think we should put this in llvm/ADT as it does not seem specific to debugging, and could be useful for others.

Also I think it doesn't need to be specific to member variables. We could just have

template <typename T> class Lazy {
  std::function<T()> Update;
  Optional<T> Value;
public:
  LazyValue(std::function<T()> Update) : Update(std::move(Update)) {}
};

Then, to get the same member functionality, you could just say:

struct Foo {
  Foo() : LazyBool([this]() { return expensiveComputeBool(); }) {}

  Lazy<bool>  LazyBool;

  bool expensiveComputeBool();
};
shafik added a subscriber: shafik.Aug 31 2018, 2:23 PM
shafik added inline comments.
include/lldb/DataFormatters/ValueObjectPrinter.h
146

I feel like using ... = is cleaner

labath added a subscriber: labath.Sep 1 2018, 4:41 AM

I've been wanting to implement something like this for a long time, so thank you for working on this. :)

Also I think it doesn't need to be specific to member variables. We could just have

template <typename T> class Lazy {
  std::function<T()> Update;
  Optional<T> Value;
public:
  LazyValue(std::function<T()> Update) : Update(std::move(Update)) {}
};

I think the issue with this approach is size. std::function is huge and heap-allocated, whereas this implementation is the same size as Optional<T> (which might still be too much for some of our use cases -- in some classes we store a bunch of needs_update flags as bitfields to conserve space, which would not be possible here).

include/lldb/DataFormatters/ValueObjectPrinter.h
138–144

I wouldn't call these update as they don't actually update anything and just return the value. In other parts of the codebase we use compute for functions like this.

146

I am actually tempted to have a macro which would define all of this boilerplate for you. :D

Something like

#define LAZY_MEMBER(Class, Type, Name) \
public: \
  Type get ## Name() { return m_ ## Name.get(*this); } \
private: \
  LazyMember<Type, Class, &Class::compute ## Name> m_ ## Name; \
  Type compute ## Name();

Then all that would be left for the user is to define the compute function in the cpp file.

include/lldb/Utility/Lazy.h
25–28

if you use Optional<T> instead of hand-rolling the flag here, you could probably get rid of the "trivial type" limitation.

teemperor updated this revision to Diff 163741.Sep 3 2018, 11:36 AM
teemperor marked an inline comment as done.
  • Now using a typedef instead of a macro.
  • Using LLVM code style in Lazy.h
  • Renamed UpdateX to CalculateX

Very nice. LGTM

teemperor marked 4 inline comments as done.Sep 11 2018, 7:34 AM

(Phabricator didn't send my draft, sorry for the delay).

I have more similar patches coming up for the other uses of LazyBool, so I would prefer finishing the implementation in LLDB and then afterwards asking if the ADT folks want it. I moved this patch to LLVM code style to make this easier later on. Sounds good?

To add to @labath's points about performance: If we have the calc* functions as lambdas, then I would have to declare all these lambdas in the constructor, which doesn't look very nice.

include/lldb/Utility/Lazy.h
25–28

Makes sense, thanks!