Repository navigation
:= doesn't always assign in-place #1248
Description
Activity
# also good dt[1:.N, a := a + a]Since I assume we all would agree that
dt[TRUE,...]should be equivalent todt[ , ...]a super-simple patch for this is addingif (missing(i)) i = TRUEat the top of[.data.tableor better yet, settingTRUEto be the default value ofiand getting rid of themissingchecks.Of course
assignwould also need to assume thatialways exists/have slightly modified logic for changing column types.The cure of switching to dt[TRUE, ...] turns out to be significantly worse than the problem with dt[ , ...]. With nrow(dt) == N, if i is missing we do not update in place, but we only do a single 8N byte allocation. With i=TRUE, R 3.2.1 with current Github master does a discouraging 28N bytes of allocation!
I'll file a separate bug for this, but if the goal is avoiding unnecessary copies and allocations and in-place changes are not a requirement, using dt[TRUE, ...] is not an acceptable workaround. Or rather, while i=TRUE should be made equivalent to a missing i (or vice versa), they both need to be fixed.
Concentrating here only on the case of a missing i, the 8N allocation we want to avoid is at line 1196:
jval = eval(jsub, SDenv, parent.frame())Since this is happening in an eval(), presumably the problem is that the column being changed has multiple references? Or had them in the past and base R is being conservative and copying first?
@nkurz I have no idea what the 3.5x allocation is about - please file a separate issue for that. For this particular issue I have a semi-working version now, but all of the modifications are in the C code.
I gave up on the literal
i=TRUEidea, because it would slow down the missingicase quite a bit. From my pov as long as missingiis exactly same asi=TRUE(or any other complicated expression that selects all of thedata.table), everything is fine, and it's irrelevant what the underlying code actually does.I filed the other bug at #1249. While it is true that a literal i=TRUE would slow down the current code, it's a very clear approach. The cleanest solution might be to coerce to i=TRUE, and then make that a fast path never creating the full logical vector. I know C much better than I know R, and I can test your fix, but my lack of familiarity with the internals of R prevents me from proposing any fixes myself.
Yes, I agree that would be cleaner, and generally speaking
[.data.tableis long overdue for a complete rehaul and cleanup, but I'm not going to volunteer for that job just yet :) My patch is going to be somewhat orthogonal to havingi=TRUEbeing the default, so that can be added at a later point.Well spotted. As suggested, fixed by converting i=TRUE to i=missing.
Instead of 5 column allocatons, there's just one now for the
a+aexpression (the RHS, which gets created anyway) which is then plonked into the column slot by reference i.e. address(DT) doesn't change but address(DT$a) will change. That's correct behaviour, and most efficient, to save copying the whole RHS into the existing column (which is only possible if they're the same type anyway). Since the RHS is as long as the number of rows, it is just plonked in. There's more on plonk in?":=".Closing for now but please reopen if I missed something.
