DEP-0007 Type-Safe Limited Collections #622

BarAgent · 2013-12-19T07:49:22Z

From the DEP-0007 document (http://opendylan.org/proposals/dep-0007.html):

To improve type safety of collections and particularly limited collections, this DEP adds a default-fill: keyword argument to limited collection classes and an element-type-fill and element-type getter to all collections.

The reference implementation is now complete and tested and is ready to be accepted or reject per DEP guidelines.

waywardmonkeys · 2013-12-20T07:22:49Z

This needs rebasing forward due to some recent merges.

BarAgent · 2013-12-21T00:59:49Z

Rebased.

hannesm · 2014-01-27T23:44:09Z

this needs some more rebasing... sorry I didn't look in here before, and just cleaned up dfmc/optimization, management, flow-graph, debug-back-end... should be straightforward to rebase... I'll look into that after I got my visualization branch up and running again... (which is wip, #660, and a (currently) local branch).

BarAgent · 2014-01-28T02:59:57Z

Rebased.

hannesm · 2014-01-30T20:53:15Z

now that the visualization has landed, could you please rebase once more?

BarAgent · 2014-01-31T00:54:27Z

Rebased.

hannesm · 2014-02-10T18:30:48Z

from the DEP, which I just read:
element-type-fill on an instance of a non-fillable collection class returns #f (as opposed to signaling an error). -- why that? I'd be happier if we error in that situation. throwing an error early leads to earlier detection of bad programs...

hannesm · 2014-02-10T18:32:06Z

and unfortunately this pull request needs yet another rebase.. I'll continue to look into the actual code and hope to merge it soon. @housel do you have an opinion on DEP7, and/or these commits here?

hannesm · 2014-02-10T18:38:20Z

sources/dfmc/typist/typist-types.dylan

@@ -842,6 +843,7 @@ define as-type-estimate-rules
                                          class:          ^limited-collection-class(t),
                                          concrete-class: ^limited-collection-concrete-class(t),
                                          of:             as(<type-estimate>, ^limited-collection-element-type(t)),
+                                          // *** Should add element-type-fill?


well, it should not be relevant for the typist, or should it?

I don't see how it would be relevant, but I'm not the typist expert. I'll go ahead and remove that comment.

BarAgent · 2014-02-11T02:08:47Z

This is because, as I mention in the element-type-fill doc, the user can introduce a fill: keyword in a custom subclass of .

element-type-fill is applicable to all collections, not just the limited collections implemented as part of OD, because it is may be necessary when processing a collection into something new, or layering something on top of another collection. So I think it is reasonable for the developer of, say, a or class to support an element-type-fill method to put it on the same footing as .

The question is how to distinguish whether a given collection is “fillable” in the abstract sense. The guide I used in the DEP is whether the class supports the fill: init-keyword. But there isn’t really a way to tell if that keyword is supported for an arbitrary class.

On Feb 10, 2014, at 10:30 AM, Hannes Mehnert [email protected] wrote:

from the DEP, which I just read:
element-type-fill on an instance of a non-fillable collection class returns #f (as opposed to signaling an error). -- why that? I'd be happier if we error in that situation. throwing an error early leads to earlier detection of bad programs...

—
Reply to this email directly or view it on GitHub.

hannesm · 2014-02-11T17:01:58Z

could you compile the dylan library as it is in master and count the generic calls (by passing -dfm to dylan-compiler and grep -c generic build/_/_dfm), and compare that to a new dylan library compiled with the new compiler, please? just to get some hints about the accumulated changes..

BarAgent · 2014-02-15T02:48:50Z

Compiling a simple app results in a build of the dylan library. In the DEP-0007 branch, that dylan library would be the DEP-0007 one, and in master, that dylan library would be the master one. They are different, so this isn't a strictly apples-to-apples comparison. With that caveat:

The master dylan library compiled with the master compiler has 3019 grep results for "generic".

The DEP-0007 dylan library compiled with the DEP-0007 compiler has 3190 grep results for "generic". Oddly, the most recent code review check-ins actually increased the number of generic calls. An earlier count was 3170.

hannesm · 2014-02-17T11:42:06Z

@BarAgent thanks for the statistics.

It would be great if we can qualify the cause of the increase. My proposal is to separate the element return type changes from the rest of this pull request, and see how big the impact thereof is. Could you do that?

I'm also in favor of a diff of the DFM of all methods where a new GF dispatch was introduced...

BarAgent · 2014-02-17T18:29:04Z

The element return type specialization does not make a difference. Re-specializing it still results in a count of 3190. Which is good, because having a specialized return type on element is a flat-out bug.

(For future reference, the command I am using to do the count is fgrep -h -c generic *.dfm | awk '{s+=$1} END {print s}'.)

I also took a look at the diff in deque.dfm and string.dfm. Many of the new "generic" references seem to be new type-checks in the DEP code. There are also new copy-down methods for as-uppercase and as-lowercase. Given that brief survey, I think it might be more informative to compare the proportions of CALLg to CALLi.

The DEP-0007 branch has 2250 CALLg and 3259 CALLi (a ratio of .69, or 41% generic).
The master branch has 2100 CALLg and 3234 CALLi (a ratio of .65, or 39% generic).

hannesm · 2014-02-17T21:56:38Z

array, extras, multidimensional-array, sequence, stretchy-vector, and table look harmless.
type is a slightly worrying; deque, limited-*, and string/unicode-string definitively needs a closer look

…does not need to be the element type of the collection if “default:” is used. Collections test suite was too strict with add! and remove!: OD implements them on <collection>, not just <sequence>.

…and repeated slots. “-with-fill” classes removed. More correct array handling in library code. Corrected implementation of “element” to allow element-not-found default of different class than element type. Removed default value for “default-fill:” keyword argument to “limited”. Neither the DRM nor the DEP calls for it.

Correctly initializes element-type-fill of concrete limited collection classes. Fixes <stretchy-vector> resizing to employ the element-type-fill when increasing collection size. Creating limited <string> results in limited <byte-string>. Removed redundant code. Added tests.

More precise fill: keyword defaults for limited collections. Declare missing shared symbol.

Removes fill value defaults from DFMC limited types; now they match the run-time limited types. Adds additional type-checking.

Resolves a case where user needed to set a default-fill when he shouldn’t have needed to.

BarAgent · 2014-02-18T05:00:51Z

Shaved off about 50 CALLg's. It all looks good to me. The remaining new CALLg's seem to be from copy-downs, additional type-checking that should have been in there in the first place, and the mechanism for picking between the new concrete limited collection classes.

hannesm · 2014-02-18T09:22:19Z

could you elaborate on that with a new set of statistics, please?

BarAgent · 2014-02-18T13:32:10Z

Here are the current differences:

DEP CALLg counts

array.dfm:36
class.dfm:37
deque.dfm:104
extras.dfm:2
generic-function.dfm:42
limited-array.dfm:31
limited-stretchy-vector.dfm:73
limited-vector.dfm:25
multidimensional-array.dfm:14
sequence.dfm:127
stretchy-vector.dfm:83
string.dfm:29
table.dfm:23
type.dfm:68
unicode-string.dfm:29

Master CALLg counts

array.dfm:35
class.dfm:36
deque.dfm:94
extras.dfm:0
generic-function.dfm:40
limited-array.dfm:22
limited-stretchy-vector.dfm:48
limited-vector.dfm:20
multidimensional-array.dfm:12
sequence.dfm:114
stretchy-vector.dfm:80
string.dfm:15
table.dfm:25
type.dfm:67
unicode-string.dfm:14

Mostly the same, but limited-vector.dfm is improved, string.dfm and unicode-string.dfm are slightly improved, and type.dfm is almost back to where it was in master. There are a few new CALLg occurrences in class.dfm, generic-function.dfm, and sequence.dfm, where CALLg replaces APPLYg, but that does not change the dispatch situation.

hannesm · 2014-02-18T18:23:40Z

I'd be more happy if the string/unicode-string and limites-array/limited-stretchy-vector are further reduced... and will look into generic-function just to make sure it is fine.

BarAgent · 2014-02-19T01:55:48Z

I've looked over limited-stretchy-vector.dfm a couple times. The new CALLg are literally all make(<type-error>), and correct implementations of type-for-copy, and a copy-down of remove — aside from stretchy-vector-element-setter which isn't inlining. And I'll take another look at that.

As for unicode-string.dfm, there is exactly one new make(<type-error>) CALLg, and everything else is simply an additional version of a function specialized on the new <limited-unicode-string> class.

hannesm · 2014-02-20T18:05:00Z

I ran a gmake 3-stage-bootstrap with both master and with your changes, timing the execution of the build process:

master: real 21m29.752s user 19m31.446s sys 1m38.824s

622: real 22m0.455s user 19m59.099s sys 1m42.149s

Thus, I'd say the performance impact is negligibly. @housel @waywardmonkeys if you don't have any showstoppers, I'm happy to merge it this sunday (23rd February).

DEP-0007 Type-Safe Limited Collections

hannesm · 2014-02-24T17:48:05Z

@BarAgent unfortunately there's a bootstrap issue: a released compiler cannot build library dylan:

Serious warning at multidimensional-array.dylan:169: 
  Reference to undefined binding {<limited-fillable-collection> in internal}. 

multidimensional-array.dylan:169: ----------------------------------------
multidimensional-array.dylan:169: define limited-array <object> (fill: #f);
multidimensional-array.dylan:169: ----------------------------------------

thus I'll revert the commit..

(old compiler [release] cannot build dylan library anymore) Revert "Merge pull request dylan-lang#622 from BarAgent/dep-0007" This reverts commit 28e4204, reversing changes made to 0132fff.

hannesm reviewed Feb 10, 2014
View reviewed changes

BarAgent added 18 commits February 17, 2014 20:51

Implementation of DEP-0007, less compatibility changes.

45d0145

Implementation of DEP-0007, with compatibility changes.

e582c86

Collections test suite was incorrect for “element”: the return value …

c1b5aff

…does not need to be the element type of the collection if “default:” is used. Collections test suite was too strict with add! and remove!: OD implements them on <collection>, not just <sequence>.

Documentation.

9f758d2

Documentation.

ae1cc98

Documentation.

8b3ce58

Moving DEP-007 release notes to next release.

97c0f5e

Only check type of make fill: keyword when it’ll be used.

ed4aab5

More precise fill: keyword defaults for limited collections. Declare missing shared symbol.

Removes remaining references to “-with-fill” classes.

13b0470

Removes fill value defaults from DFMC limited types; now they match the run-time limited types. Adds additional type-checking.

Fixes copy-sequence for limited strings & deques.

f789586

Resolves a case where user needed to set a default-fill when he shouldn’t have needed to.

Added type specializer to value of concrete-limited-X-class methods.

d9e574a

Removed misleading comments.

494353a

Inlined make-sequence, per code review.

3f15e78

Reverted unnecessarily general specializer.

533af72

Unreverted necessarily specialized generics.

e3092bd

Dispatch optimizations.

92d2d5d

hannesm added a commit that referenced this pull request Feb 24, 2014

Merge pull request #622 from BarAgent/dep-0007

28e4204

DEP-0007 Type-Safe Limited Collections

hannesm merged commit 28e4204 into dylan-lang:master Feb 24, 2014

hannesm mentioned this pull request Feb 24, 2014

Reverting due to bootstrap issues #687

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEP-0007 Type-Safe Limited Collections #622

DEP-0007 Type-Safe Limited Collections #622

BarAgent commented Dec 19, 2013

waywardmonkeys commented Dec 20, 2013

BarAgent commented Dec 21, 2013

hannesm commented Jan 27, 2014

BarAgent commented Jan 28, 2014

hannesm commented Jan 30, 2014

BarAgent commented Jan 31, 2014

hannesm commented Feb 10, 2014

hannesm commented Feb 10, 2014

hannesm Feb 10, 2014

BarAgent Feb 11, 2014

BarAgent commented Feb 11, 2014

hannesm commented Feb 11, 2014

BarAgent commented Feb 15, 2014

hannesm commented Feb 17, 2014

BarAgent commented Feb 17, 2014

hannesm commented Feb 17, 2014

BarAgent commented Feb 18, 2014

hannesm commented Feb 18, 2014

BarAgent commented Feb 18, 2014

hannesm commented Feb 18, 2014

BarAgent commented Feb 19, 2014

hannesm commented Feb 20, 2014

hannesm commented Feb 24, 2014

DEP-0007 Type-Safe Limited Collections #622

DEP-0007 Type-Safe Limited Collections #622

Conversation

BarAgent commented Dec 19, 2013

waywardmonkeys commented Dec 20, 2013

BarAgent commented Dec 21, 2013

hannesm commented Jan 27, 2014

BarAgent commented Jan 28, 2014

hannesm commented Jan 30, 2014

BarAgent commented Jan 31, 2014

hannesm commented Feb 10, 2014

hannesm commented Feb 10, 2014

hannesm Feb 10, 2014

Choose a reason for hiding this comment

BarAgent Feb 11, 2014

Choose a reason for hiding this comment

BarAgent commented Feb 11, 2014

hannesm commented Feb 11, 2014

BarAgent commented Feb 15, 2014

hannesm commented Feb 17, 2014

BarAgent commented Feb 17, 2014

hannesm commented Feb 17, 2014

BarAgent commented Feb 18, 2014

hannesm commented Feb 18, 2014

BarAgent commented Feb 18, 2014

hannesm commented Feb 18, 2014

BarAgent commented Feb 19, 2014

hannesm commented Feb 20, 2014

hannesm commented Feb 24, 2014