Use workaround to enable lifetime checking for lists of borrowed classes #15575

dlongnecke-cray · 2020-04-23T02:36:10Z

This PR adds a small workaround to the List module that "encourages" the compiler to perform lifetime checking for methods such as list.append().

This workaround is required because lists use _ddata for their backing store instead of an array. When it comes time for the compiler to evaluate lifetime clauses, a lifetime clause is only considered for evaluation if both formals in the clause are or refer to a borrowed class (the relevant function in compiler/resolution/lifetime.cpp is isOrRefersBorrowedClass). The _ddata type is marked as unsafe by the compiler, and is skipped. Because of this the compiler is unable to infer that a list(borrowed C) contains a borrowed class, and lifetime clauses with the list as a receiver such as this < x are not evaluated.

To resolve this issue, I added a dummy field to the list type. If the eltType of a list is a borrowed class, this field will have the type eltType? (a nilable eltType) and is default initialized to nil. Otherwise this field has the type nothing and is folded away.

Testing:

ALL on linux64 when CHPL_COMM=none
ALL on linux64 when CHPL_COMM=gasnet

cassella · 2020-04-27T23:28:12Z

compiler/resolution/lifetime.cpp

+            bool bothFormalsContainBorrowedOrAreBorrowed = false;       
+
+            if (isOrRefersBorrowedClass(formal1->getValType())) {
+              bothFormalsContainBorrowedOrAreBorrowed = true;


Nothing's going to set this to false when formal2 isn't borrowed.

Good point!

dlongnecke-cray · 2020-04-27T23:43:28Z

Hi @mppf, would you mind reviewing this? I added debugging info I found helpful but let me know if that is too much for you.

mppf

It looks good but I'd like you to remove the debug code. Most of that information is stuff I would gather from the debugger and tracing the checking of a particular call. It's enough code that I worry about it making the control flow of the program logic harder to see.

mppf · 2020-04-28T13:04:31Z

compiler/AST/FnSymbol.cpp

+    if (isResolved() && _this != NULL) {
+      return toAggregateType(_this->getValType());
+    } else {
+      if (numFormals() >= 2) {


Let's fold else { if { into else if {

dlongnecke-cray · 2020-04-28T16:55:01Z

Thought so. I'll go ahead and remove the debugging code. I'm not sure how to step through in the way you do with a debugger yet, I'll have to practice.

@ben-albrecht

Adjust list futures affected by merge of 15575 (#15608) This PR adjusts list lifetime checker futures that were not addressed by #15575. Some have been changed to normals tests. The test `listAppendBorrow` was broken up into two smaller, more concise tests. --- Testing: - [x] Adjusted tests on `linux64` when `CHPL_COMM=none` --- Reviewed by @ben-albrecht.

dlongnecke-cray · 2020-06-25T16:57:15Z

modules/standard/List.chpl

-      for x in iterable do
-        append(x);
+
+      for x in iterable do {


@ronawho So after profiling this PR this is trivially the reason comm counts changed in test/library/packages/Sort/performance/dist-performance.chpl. The only thing I don't understand is why. Do you think it's worth trying to figure out why this diff caused comm counts to change so drastically? Or should I just revert it and call it a day?

Do you remember the motivation for this change? I don't think I understand what's going on here well enough to have an opinion.

My rationale for this change was that since _commonInitFromIterable is only called in an initializer, I might as well write the loop using internal methods to avoid taking a lock (list.append may take a lock if parSafe=true).

I see. That makes sense, though I still don't grok the "no auto destroy"s. Can we make an internal version of append that doesn't take the lock?

proc ref _appendNoLock(pragma "no auto destroy" in x: this.eltType) lifetime this < x { _appendByRef(x); }

Or does that still have the comm issue?

If that still has the problem, I'd probably just revert and keep the lock. That lock has very little overhead when used uncontested/serially

The pragma "no auto destroy" exist because the list assumes ownership of the lifetime of the element. It controls when the element is destroyed as long as it contains it. (The element is moved into place instead of assigned, so only one deep copy is made.)

I can try _appendNoLock but honestly it's probably not worth adding a whole new method, as you say the overhead is not much when uncontested.

I was just curious about whether or not this is worth investigating (as a separate issue or something), as to me it doesn't really seem like the changes should cause such a massive increase in GETs. But maybe it is trivially obvious to you why GETs would increase?

I suspect that the pragma code confuses the compile enough that it doesn't remove a copy for the case that the iterator is returning by value. You could check the AST logs around after pass 20 callDestructors for differences to see if that's the case.

Awesome! I'll give that a try.

No luck, seems more or less the same sans an initCopy for the formal tmp in the append version vs an init= for the variable in the _appendByRef version. Iterator was returning refs.

@ronawho

Revert changes to `list._commonInitFromIterable` (#15943) In #15575 I decided to modify `list._commonInitFromIterable` to append elements without taking a lock. Such a small optimization seemed reasonable since that method is only called by initializers. The change ended up causing a fairly large increase (30270 -> 34410) in GETs for `library/packages/Sort/performance/dist-performance`, so revert it. Add a lifetime constraint to `_commonInitFromIterable` so that old code can function correctly. --- Testing: - [x] ALL on linux64 when CHPL_COMM=none --- Reviewed by @ronawho. Thanks!

@ronawho

Revert changes to `list._commonInitFromIterable` (chapel-lang#15943) In chapel-lang#15575 I decided to modify `list._commonInitFromIterable` to append elements without taking a lock. Such a small optimization seemed reasonable since that method is only called by initializers. The change ended up causing a fairly large increase (30270 -> 34410) in GETs for `library/packages/Sort/performance/dist-performance`, so revert it. Add a lifetime constraint to `_commonInitFromIterable` so that old code can function correctly. --- Testing: - [x] ALL on linux64 when CHPL_COMM=none --- Reviewed by @ronawho. Thanks!

dlongnecke-cray added 8 commits April 17, 2020 13:25

Remove list check for borrowed

832a5b4

Add tentative future, may not actually be a bug

00bce1c

Adjust list test to use where clause

4520842

Convert passing futures to regular tests

48471fa

Add future demonstrating problem with list.append

753a56a

Freeze before testing 1.19

8b503b2

All of a sudden, error for append constraint is firing?

083b9bd

Add clever workaround to List module

7f38bcb

dlongnecke-cray self-assigned this Apr 23, 2020

dlongnecke-cray mentioned this pull request Apr 23, 2020

Supported types in collections #15394

Closed

6 tasks

dlongnecke-cray added 6 commits April 22, 2020 19:40

Small cleanups in List module, adjust test

7439be6

Turn off lifetime debugging, add sanitizer for macro

63084c4

Adjust .good file

bc01905

Warn directive -> warning

e19d7cb

Adjust indents, move comment

9b9493a

Remove future that is no longer relevant

c238464

ben-albrecht mentioned this pull request Apr 23, 2020

Address issues with collection x types tests #15574

Merged

dlongnecke-cray added 6 commits April 27, 2020 12:13

Remove debugf macro

bfaa481

Merge branch 'master' into fix-lists-of-borrowed

75914b1

Add variant to tests, adjust module comment

1243061

Remove line numbers from test

ad5c77d

Add comment to test

ec2b736

Do not take locks in common initializer

cc28afa

cassella reviewed Apr 27, 2020

View reviewed changes

Fix bad boolean logic, thanks @cassella

85253ab

dlongnecke-cray requested a review from mppf April 27, 2020 23:42

Adjust .good file

17363a0

mppf approved these changes Apr 28, 2020

View reviewed changes

dlongnecke-cray added 5 commits April 28, 2020 10:01

Remove debugging blocks

4c4aa09

Add accidentally removed code from diffs

54aebe8

Merge else if branches together

783a050

Remove trailing spaces

d75d7aa

Trailing space

4b2bdd3

dlongnecke-cray merged commit fd8a023 into chapel-lang:master Apr 28, 2020

dlongnecke-cray mentioned this pull request Apr 29, 2020

Adjust list futures affected by merge of 15575 #15608

Merged

1 task

ronawho mentioned this pull request May 21, 2020

Add perf annotations for 2020-05-21 #15709

Merged

dlongnecke-cray mentioned this pull request May 27, 2020

List.append ignoring lifetime constraints when its eltType is a class #13613

Closed

dlongnecke-cray commented Jun 25, 2020

View reviewed changes

dlongnecke-cray added a commit to dlongnecke-cray/chapel that referenced this pull request Jun 25, 2020

Revert changes to _commonInitFromIterable made in chapel-lang#15575

9388c71

dlongnecke-cray mentioned this pull request Jun 25, 2020

Revert changes to list._commonInitFromIterable #15943

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use workaround to enable lifetime checking for lists of borrowed classes #15575

Use workaround to enable lifetime checking for lists of borrowed classes #15575

dlongnecke-cray commented Apr 23, 2020 •

edited

Loading

cassella Apr 27, 2020

dlongnecke-cray Apr 27, 2020

dlongnecke-cray commented Apr 27, 2020

mppf left a comment

mppf Apr 28, 2020

dlongnecke-cray commented Apr 28, 2020

dlongnecke-cray Jun 25, 2020

ronawho Jun 25, 2020

dlongnecke-cray Jun 25, 2020 •

edited

Loading

ronawho Jun 25, 2020

dlongnecke-cray Jun 25, 2020 •

edited

Loading

mppf Jun 25, 2020

dlongnecke-cray Jun 25, 2020

dlongnecke-cray Jun 25, 2020

Use workaround to enable lifetime checking for lists of borrowed classes #15575

Use workaround to enable lifetime checking for lists of borrowed classes #15575

Conversation

dlongnecke-cray commented Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlongnecke-cray commented Apr 27, 2020

mppf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlongnecke-cray commented Apr 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlongnecke-cray Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlongnecke-cray Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlongnecke-cray commented Apr 23, 2020 •

edited

Loading

dlongnecke-cray Jun 25, 2020 •

edited

Loading

dlongnecke-cray Jun 25, 2020 •

edited

Loading