Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filling array from offset can be slow #590

Closed
XmiliaH opened this issue Jul 14, 2022 · 6 comments · Fixed by #865
Closed

Filling array from offset can be slow #590

XmiliaH opened this issue Jul 14, 2022 · 6 comments · Fixed by #865
Assignees
Labels
bug Something isn't working

Comments

@XmiliaH
Copy link
Contributor

XmiliaH commented Jul 14, 2022

When filling an array from a larger offset filling the array can become very slow.

The following code finished in PUC Lua and LuaJIT in under a second while Luau needs almost 3 minutes!

local t = {}
for i = 100000,400000 do
    t[i] = 1
end

However, when beginning from index 1 all implementations are comparable.

local t = {}
for i = 1,400000 do
    t[i] = 1
end

The following table shows the timings of the two cases for different Lua implementations.

Software With Offset (Seconds) From 1 (Seconds)
Luau (Debug) 179.0418060 0.0164390
Luau (Release) 72.4990020 0.0067836
Lua 5.1 0.0320840 0.0126690
Lua 5.4 0.0271857 0.0076240
LuaJIT 0.0068230 0.0026620

The problem seems to be that at some point the insertions start to hit the case https://github.com/Roblox/luau/blob/a934f742d81aefa53e8e45fa7eec0042d3fde7fc/VM/src/ltable.cpp#L530-L536 which rehashes the whole array every time the next index is inserted.

@XmiliaH XmiliaH added the bug Something isn't working label Jul 14, 2022
@zeux
Copy link
Collaborator

zeux commented Jul 14, 2022

Yeah there's an edge case in the table invariant that we knew about, but didn't have a sufficiently motivating use case to fix. I think this only applies when the offset is large/comparable with the target array size, as is in this example.

(as an aside, your timings are a little surprising, on my system Lua 5.4 produces roughly comparable timings to Luau for the example that is starting from 1 - although if this is on macOS/M1 then yeah Lua 5.4 gets some benefits from first-class integers in programs like this, which we plan to address in the future)

@zeux zeux self-assigned this Jul 14, 2022
@XmiliaH
Copy link
Contributor Author

XmiliaH commented Jul 14, 2022

(as an aside, your timings are a little surprising, on my system Lua 5.4 produces roughly comparable timings to Luau for the example that is starting from 1 - although if this is on macOS/M1 then yeah Lua 5.4 gets some benefits from first-class integers in programs like this, which we plan to address in the future)

I use x86_64. This could be caused by building Luau with just make which should produce a debug build which ought to be slower. However, the timings were only included to show that in normal cases all timings are roughly the same.

@zeux
Copy link
Collaborator

zeux commented Jul 14, 2022

Ah, gotcha - yeah, makes sense in this case. There's a few simple tweaks to table resize that eliminate this edge case, so we'll take a look - thanks!

@vegorov-rbx vegorov-rbx self-assigned this Mar 9, 2023
@vegorov-rbx
Copy link
Collaborator

I have a few adjustments ready that will bring time back to 10s of milliseconds.

@vegorov-rbx
Copy link
Collaborator

Ok, it seems that it's harder to get right than I expected, so probably won't have a fix.

@vegorov-rbx
Copy link
Collaborator

Found the right fix, but unfortunately it will miss this weeks sync.

@zeux zeux assigned vegorov-rbx and unassigned zeux Mar 13, 2023
vegorov-rbx added a commit that referenced this issue Mar 17, 2023
* A small subset of control-flow refinements have been added to
recognize type options that are unreachable after a
conditional/unconditional code block. (Fixes
#356).

Some examples:
```lua
local function f(x: string?)
    if not x then return end

    -- x is 'string' here
end
```
Throwing calls like `error` or `assert(false)` instead of 'return' are
also recognized.
Existing complex refinements like type/typeof and tagged union checks
are expected to work, among others.

To enable this feature, `LuauTinyControlFlowAnalysis` exclusion has to
be removed from `ExperimentalFlags.h`.
If will become enabled unconditionally in the near future.

* Linter has been integrated into the typechecker analysis so that
type-aware lint warnings can work in any mode
`Frontend::lint` methods were deprecated, `Frontend::check` has to be
used instead with `runLintChecks` option set.
Resulting lint warning are located inside `CheckResult`.

* Fixed large performance drop and increased memory consumption when
array is filled at an offset (Fixes
#590)
* Part of [Type error suppression
RFC](https://github.com/Roblox/luau/blob/master/rfcs/type-error-suppression.md)
was implemented making subtyping checks with `any` type transitive.

---
In our work on the new type-solver:
* `--!nocheck` mode no longer reports type errors
* New solver will not be used for `--!nonstrict` modules until all
issues with strict mode typechecking are fixed
* Added control-flow aware type refinements mentioned earlier

In native code generation:
* `LOP_NAMECALL` has been translated to IR
* `type` and `typeof` builtin fastcalls have been translated to
IR/assembly
* Additional steps were taken towards arm64 support
RomanKhafizianov pushed a commit to RomanKhafizianov/luau that referenced this issue Nov 27, 2023
* A small subset of control-flow refinements have been added to
recognize type options that are unreachable after a
conditional/unconditional code block. (Fixes
luau-lang/luau#356).

Some examples:
```lua
local function f(x: string?)
    if not x then return end

    -- x is 'string' here
end
```
Throwing calls like `error` or `assert(false)` instead of 'return' are
also recognized.
Existing complex refinements like type/typeof and tagged union checks
are expected to work, among others.

To enable this feature, `LuauTinyControlFlowAnalysis` exclusion has to
be removed from `ExperimentalFlags.h`.
If will become enabled unconditionally in the near future.

* Linter has been integrated into the typechecker analysis so that
type-aware lint warnings can work in any mode
`Frontend::lint` methods were deprecated, `Frontend::check` has to be
used instead with `runLintChecks` option set.
Resulting lint warning are located inside `CheckResult`.

* Fixed large performance drop and increased memory consumption when
array is filled at an offset (Fixes
luau-lang/luau#590)
* Part of [Type error suppression
RFC](https://github.com/Roblox/luau/blob/master/rfcs/type-error-suppression.md)
was implemented making subtyping checks with `any` type transitive.

---
In our work on the new type-solver:
* `--!nocheck` mode no longer reports type errors
* New solver will not be used for `--!nonstrict` modules until all
issues with strict mode typechecking are fixed
* Added control-flow aware type refinements mentioned earlier

In native code generation:
* `LOP_NAMECALL` has been translated to IR
* `type` and `typeof` builtin fastcalls have been translated to
IR/assembly
* Additional steps were taken towards arm64 support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

3 participants