Making Stacktrace immutable, so it can be hashable

Making Stacktrace immutable, so it can be hashable

To use loglinear models for classifying CLs, we need CallStack and
Stacktrace to be valid argument types for MemoizedFunction, which means
they must be hashable, which means they must be immutable. This CL makes
Stacktrace immutable.

BUG=
TBR=stgao@chromium.org

Review-Url: https://codereview.chromium.org/2557423003
Committed: https://chromium.googlesource.com/infra/infra/+/805ebac8bd97020ba6072a4d344ac6c2fd63338e

[wrengr, 2016-12-08 19:55:04]

Description was changed from

==========
Making Stacktrace immutable, so it can be hashable

To use loglinear models for classifying CLs, we need CallStack and
Stacktrace to be valid argument types for MemoizedFunction, which means
they must be hashable, which means they must be immutable. This CL makes
Stacktrace immutable.

BUG=
TBR=stgao@chromium.org
==========

to

==========
Making Stacktrace immutable, so it can be hashable

To use loglinear models for classifying CLs, we need CallStack and
Stacktrace to be valid argument types for MemoizedFunction, which means
they must be hashable, which means they must be immutable. This CL makes
Stacktrace immutable.

BUG=
TBR=stgao@chromium.org
==========

[wrengr, 2016-12-08 19:55:04]

wrengr@chromium.org changed reviewers:
+ inferno@chromium.org, katesonia@chromium.org, mbarbella@chromium.org
- stgao@chromium.org

[wrengr, 2016-12-08 19:55:31]

Second half of making CallStack and Stacktrace immutable. PTAL

[inferno, 2016-12-08 20:52:15]

lgtm

[Martin Barbella, 2016-12-08 22:21:03]

lgtm

[Sharu Jiang, 2016-12-08 22:45:48]

I have same concern as comments in https://codereview.chromium.org/2562623004/

I think it's not necessary to make every parameter to memorized function
immutable. If you use the cache decorator in
https://cs.chromium.org/chromium/infra/appengine/findit/libs/cache_decorator.py,
we have something like _DefaultKeyGenerator(You can define yours to serve your
purpose) to generate key.

Namely, in-memory cache subclass + customized KeyGenerator can do the same thing
as MemorizedFunction and do not need to change the Callstack and StackTrace from
list to namedtuple.
 
Another concern is that we do a lot manipulation to CallStack and StackTrace,
e.g. a lot filtering. Making it immutable will cost the efficiency.

[wrengr, 2016-12-08 23:46:50]

On 2016/12/08 22:45:48, Sharu Jiang wrote:
> I have same concern as comments in https://codereview.chromium.org/2562623004/
> 
> I think it's not necessary to make every parameter to memorized function
> immutable. If you use the cache decorator in
>
https://cs.chromium.org/chromium/infra/appengine/findit/libs/cache_decorator.py,
> we have something like _DefaultKeyGenerator(You can define yours to serve your
> purpose) to generate key.
> 
> Namely, in-memory cache subclass + customized KeyGenerator can do the same
thing
> as MemorizedFunction and do not need to change the Callstack and StackTrace
from
> list to namedtuple.
>  
> Another concern is that we do a lot manipulation to CallStack and StackTrace,
> e.g. a lot filtering. Making it immutable will cost the efficiency.

Sure, we could make it hashable by generating unique keys per instance, but that
seems gross to me. In practice we don't actually mutate stacks and traces, nor
should we. The only manipulation we do is SliceFrames; and while the current
version is naive/obvious, it can be made to work in constant time if we really
do encounter performance problems.

[Sharu Jiang, 2016-12-09 00:59:43]

On 2016/12/08 23:46:50, wrengr wrote:
> On 2016/12/08 22:45:48, Sharu Jiang wrote:
> > I have same concern as comments in
https://codereview.chromium.org/2562623004/
> > 
> > I think it's not necessary to make every parameter to memorized function
> > immutable. If you use the cache decorator in
> >
>
https://cs.chromium.org/chromium/infra/appengine/findit/libs/cache_decorator.py,
> > we have something like _DefaultKeyGenerator(You can define yours to serve
your
> > purpose) to generate key.
> > 
> > Namely, in-memory cache subclass + customized KeyGenerator can do the same
> thing
> > as MemorizedFunction and do not need to change the Callstack and StackTrace
> from
> > list to namedtuple.
> >  
> > Another concern is that we do a lot manipulation to CallStack and
StackTrace,
> > e.g. a lot filtering. Making it immutable will cost the efficiency.
> 
> Sure, we could make it hashable by generating unique keys per instance, but
that
> seems gross to me. In practice we don't actually mutate stacks and traces, nor
> should we. The only manipulation we do is SliceFrames; and while the current
> version is naive/obvious, it can be made to work in constant time if we really
> do encounter performance problems.

Since we are commenting in 2 different cls about the same thing, to simplify
things, I just respond in https://codereview.chromium.org/2562623004/ to make it
in one place :)

[Sharu Jiang, 2016-12-09 20:49:23]

lgtm

[wrengr, 2016-12-12 17:55:06]

The CQ bit was checked by wrengr@chromium.org

[wrengr, 2016-12-12 17:55:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
mbarbella@chromium.org, inferno@chromium.org
Link to the patchset: https://codereview.chromium.org/2557423003/#ps20001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-12 17:55:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 17:55:12]

This CL has an open dependency (Issue 2562623004 Patch 60001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[wrengr, 2016-12-12 18:34:01]

The CQ bit was checked by wrengr@chromium.org

[commit-bot: I haz the power, 2016-12-12 18:34:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 18:48:41]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481567641284220,
"parent_rev": "172ff229a8b2490626f58d90b4d2904417c22a4c", "commit_rev":
"805ebac8bd97020ba6072a4d344ac6c2fd63338e"}

[commit-bot: I haz the power, 2016-12-12 18:48:44]

Description was changed from

==========
Making Stacktrace immutable, so it can be hashable

To use loglinear models for classifying CLs, we need CallStack and
Stacktrace to be valid argument types for MemoizedFunction, which means
they must be hashable, which means they must be immutable. This CL makes
Stacktrace immutable.

BUG=
TBR=stgao@chromium.org
==========

to

==========
Making Stacktrace immutable, so it can be hashable

To use loglinear models for classifying CLs, we need CallStack and
Stacktrace to be valid argument types for MemoizedFunction, which means
they must be hashable, which means they must be immutable. This CL makes
Stacktrace immutable.

BUG=
TBR=stgao@chromium.org

Review-Url: https://codereview.chromium.org/2557423003
Committed:
https://chromium.googlesource.com/infra/infra/+/805ebac8bd97020ba6072a4d344ac...
==========

[commit-bot: I haz the power, 2016-12-12 18:48:45]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/infra/infra/+/805ebac8bd97020ba6072a4d344ac...