Introduce View[T] type #5957

zielmicha · 2017-06-07T14:06:22Z

(rationale available as RFC #5753)

This PR adds views module together with View[T] type. It is defined as a triple:

type View[T] = 
  data: ptr T
  length: int
  when needGcKeep: # not needed for gc:none and gc:boehm
    gcKeep: RootRef

initView creates view pointing into existing memory, newView creates view by copying seq or string
copyAsSeq, copyAsString convert the view to string/seq
slice takes a subview of a view
copyFrom, copyTo copy data from views around
[], []=, iterator items are available

yglukhov · 2017-06-07T14:21:11Z

lib/pure/views.nim

+
+proc newView*(s: string): ByteView =
+  ## Copies a string and returns a new view pointing into the copy.
+  let copied = new(string)


Why copy string? It could be shallowCopied instead. And no need for ref string variant, IMO

I'm not sure. shallowCopy is counter-intuitive here. From system docs:

Be careful with the changed semantics though! There is a reason why the default assignment does a deep copy of sequences and strings.

Also: notice that ref string variant is not exported, because it's not safe when the string is resized.

yglukhov · 2017-06-07T14:21:47Z

This implementation looks completely unfriendly to JS

zielmicha · 2017-06-07T14:30:20Z

Hmm, I guess JS needs separate implementation. At least fast C interop doesn't apply there.

zielmicha · 2017-06-11T15:52:07Z

This implementation looks completely unfriendly to JS

@yglukhov Is JS backend mature enough to consider implementing views for it? Previous time I checked (~two years ago) it wasn't possible to write nontrivial program using it and now I failed too (#5974).

krux02 · 2017-07-24T18:50:08Z

Have you thought about supporting the slicing notation? myseq.initView(0..^1)

zielmicha · 2017-07-24T21:16:20Z

Good idea. Maybe I should just add [] operator for slices (so initView(myseq)[0..^1]).

zielmicha · 2017-09-26T10:42:03Z

@dom96 Do you think this can be merged? Any specific comments?

yglukhov · 2017-09-26T11:10:22Z

@zielmicha, actually I've got a philosophical comment regarding the gcKeep field. I think in the very base impl of View there should be no gcKeep to reduce the overhead even further. I don't want to stall this PR by any means, because its already better than nothing, but can we do anything about it still keeping the api safe by default, and unsafe as an option?

krux02 · 2017-07-24T18:44:30Z

lib/pure/views.nim

+    data: ptr T
+    length: int
+    when needGcKeep:
+      gcKeep: RootRef


I think that you need both types, the type that keeps the underlying datastructure alive and one that doesn't. I expect a View object to be passed down the call stack, but it would not be written to some variable. So It would not need to keep the original object alive. The need for a gcKeep would be an exceptional case. But your implementation only allows to have it either globally enabled or disabled.

needGcKeep is defined as not (compileOption("gc", "boehm") or compileOption("gc", "none")), so it can't be disabled even globally.

yes that is what I mean. It can be disabled/enabled globally. But it should be possible to enable/disable it locally. Meaning a different type that has the RootRef and one that does not. Similar to ptr/ref being a different type. One does keep the object alive the other one doesn't.

krux02 · 2017-07-24T18:48:19Z

lib/pure/views.nim

+  # View[char] and View[byte] are mostly the same thing
+  return initView(cast[ptr byte](s.data), s.len, when needGcKeep: s.gcKeep else: nil)
+
+proc initView(s: ref string): ByteView =


both initView that accept string as well as seq only allow this, when the seq/string is a ref. To my knowledge it is not safe to cast a ptr to a ref, therefore it is not possible to "view" into a seq that is not ref type.

krux02 · 2017-07-24T22:02:42Z

lib/pure/views.nim

+  return v.length
+
+proc ptrAdd[T](p: pointer, i: int): ptr T =
+  return cast[ptr T](cast[int](p) +% (i * sizeof(T)))


why don't you use a pointer to an unchecked array? That is a pointer with pointer arithmetics.

zielmicha · 2017-09-26T22:05:02Z

@yglukhov The only cost is that passing View as an argument copies 24 bytes, not 16. I'm not sure if introducing two types is worth this small saving.

At least in my usecases, I haven't observed any performance difference at all between 24 and 16 byte View (reactor.nim uses View from collections.nim).

dom96 · 2017-10-23T13:49:16Z

I would prefer the name Slice but I guess that is already taken.

Otherwise I am going to delegate this to @Araq, I'm mostly happy to include this though.

dom96 · 2017-10-23T13:51:46Z

lib/pure/views.nim

+
+  ByteView* = View[byte]
+
+proc initView*[T; R: ref](data: ptr T, len: int, gcKeep: R): View[T] =


I think that any function which accepts a 'ptr' should have an unsafe prefix.

dom96 · 2017-10-23T13:52:03Z

lib/pure/views.nim

@@ -0,0 +1,148 @@
+## View is a type representing a range of elements in an array. In can be thought as a pointer plus a size.


In -> It

dom96 · 2017-10-23T13:52:16Z

lib/pure/views.nim

+## View is a type representing a range of elements in an array. In can be thought as a pointer plus a size.
+## View can be created to an arbitrary memory segment and can additionally keep a single ``ref`` object alive.
+##
+## This module defines views and several helper operations on them. All functions in this module (except for unsafe version of ``initView``) are designed to be memory safe (e.g. they raise exception on out-of-bounds accesses).


Examples in the docs would be nice.

dom96 · 2017-10-23T13:53:13Z

lib/pure/views.nim

+  else:
+    initView(addr s[0], s[].len, gcKeep=s)
+
+proc newView*[T](s: seq[T]): View[T] =


This doesn't follow Nim's convention.

dom96 · 2017-10-23T13:53:23Z

lib/pure/views.nim

+  ## Creates a view of length zero.
+  return initView[T](nil, 0)
+
+proc initView[T](s: ref seq[T]): View[T] =


Shouldn't this be var seq[T]?

That wouldn't be memory safe. (You can use initView(ptr T, int) for that)

Couldn't you use GC_ref to make it safe?

Yes. But I think then it needs to allocate object to make it possible to call GC_unref in destructor.

dom96 · 2017-10-23T13:54:03Z

lib/pure/views.nim

+  result = initView(s)
+
+proc isNil*(v: View): bool =
+  return v.len == 0


This is rather misleading.

Also, how can you use 'len' here when it's defined below this function?

View is generic and len has multiple overloads, so it's mixin by default.

I would say, a View is never Nil, just empty and therefore this isNil should be removed.

Araq · 2018-01-11T10:39:16Z

Stylistic issues aside, it's mostly fine but I think it would be ill advised to introduce this. Once openArray and var openArray work with some minor Rust inspired borrow checking to make it safe (RFC soon to be written) there would be considerable overlap with this stdlib module.

zielmicha · 2018-01-11T14:29:13Z

It's great the hear that the are plans to create first-class openArray. Then I think we can close this for now.

yglukhov reviewed Jun 7, 2017

View reviewed changes

introduce View[T] type

1cc5167

zielmicha force-pushed the new-io branch from edf34c6 to 1cc5167 Compare June 7, 2017 15:25

krux02 reviewed Sep 26, 2017

View reviewed changes

dom96 reviewed Oct 23, 2017

View reviewed changes

nitely mentioned this pull request Nov 7, 2017

Make iterator zero-copy nitely/nim-graphemes#4

Closed

zielmicha closed this Jan 11, 2018

This was referenced Jul 10, 2018

[WIP] nim needs a slice type analag to D's T[] dynamic array #8256

Closed

ICE's and bugs with openarray #8259

Closed

timotheecour mentioned this pull request Jan 13, 2019

make openArray first-class nim-lang/RFCs#88

Closed

mratsim mentioned this pull request Nov 7, 2019

lent/var/openArray inside objects/containers nim-lang/RFCs#178

Closed


		ByteView* = View[byte]

		proc initView*[T; R: ref](data: ptr T, len: int, gcKeep: R): View[T] =

		@@ -0,0 +1,148 @@
		## View is a type representing a range of elements in an array. In can be thought as a pointer plus a size.

Uh oh!

Introduce View[T] type #5957

Introduce View[T] type #5957

Uh oh!

Conversation

zielmicha commented Jun 7, 2017

Uh oh!

yglukhov Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zielmicha Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yglukhov commented Jun 7, 2017

Uh oh!

zielmicha commented Jun 7, 2017

Uh oh!

zielmicha commented Jun 11, 2017

Uh oh!

krux02 commented Jul 24, 2017

Uh oh!

zielmicha commented Jul 24, 2017

Uh oh!

zielmicha commented Sep 26, 2017

Uh oh!

yglukhov commented Sep 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zielmicha commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dom96 commented Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krux02 Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Araq commented Jan 11, 2018

Uh oh!

zielmicha commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

yglukhov Jun 7, 2017 •

edited

Loading

zielmicha Jun 7, 2017 •

edited

Loading

zielmicha commented Sep 26, 2017 •

edited

Loading

dom96 commented Oct 23, 2017 •

edited

Loading

krux02 Oct 24, 2017 •

edited

Loading

zielmicha commented Jan 11, 2018 •

edited

Loading