Juno.jl icon indicating copy to clipboard operation
Juno.jl copied to clipboard

[todo] refactor out auto module detection handlings

Open aviatesk opened this issue 6 years ago • 2 comments

The discussion has continued from https://github.com/JunoLab/Atom.jl/pull/192#issuecomment-536489355.

In short, we now have 3 alternatives to detect a file's module:

  1. old way based on CodeTools: https://github.com/JunoLab/Atom.jl/blob/9cf10173e4c5727451e43950872d6cdb3795c720/src/eval.jl#L13-L39
  2. Revise-like approach: https://github.com/JunoLab/Atom.jl/blob/9cf10173e4c5727451e43950872d6cdb3795c720/src/utils.jl#L175-L359
  3. CSTParser-based approach: https://github.com/JunoLab/Atom.jl/blob/9cf10173e4c5727451e43950872d6cdb3795c720/src/goto.jl#L185-L205

1 is being used for module handler, 2 and 3 are being used for the recent goto (and renamerefactor, in the future) handler.

The main purpose of this refactor is to implement a more robust module detection logic in the module handler by replacing 1. with something like a combination of 2. and 3. Or at least, since 1. and 3. are kinda duplicated works, so we want to unify the logic. (and this will hopefully leads to less JIT compiles)

We may need to significantly refactor the frontend code as well -- it's old too.

@pfitzseb can you bundle the related issues here (link to an issue report if exists or just list the random confirmed cases), so that we can easily check if we could address them by this refactor ?

P.S. Feel free to edit this issue for the future reference. We want more structured information for this.

aviatesk avatar Oct 25 '19 01:10 aviatesk

https://github.com/JunoLab/Atom.jl/pull/210 serves as a preparation for this.

aviatesk avatar Oct 29 '19 09:10 aviatesk

A little interesting fact on this: both modulefiles(mod::Module) and CSTParser-based module traverse runs in almost the same amount of time:

I changed these lines into

const profile_id = Ref{Int}(0)

function _collecttoplevelitems(mod::Module)
  profile_id[] += 1
  return if profile_id[] % 2 === 0
    # Revise-like approach
    entrypath, paths = modulefiles(mod)
    __collecttoplevelitems(stripdotprefixes(string(mod)), [entrypath; paths])
  else # CSTParser-based approach
    entrypath, line = moduledefinition(mod)
    __collecttoplevelitems(stripdotprefixes(string(mod)), entrypath)
  end
end

and precompiled Atom module (to make sure modulefile works for Atom module), and then profiled globalgotoitems like below

Juno.@profiler begin
  for i = 1:30
    Atom.globalgotoitems("SYMBOLSCACHE", Atom, nothing, "")
    Atom.clearsymbols()
  end
end

, which would cause both Revise-like module file finding and CSTParser-based module traverse in turn.

And the profiling yielded that almost most same amount of time were spent on entrypath, paths = modulefiles(mod) and __collecttoplevelitems(stripdotprefixes(string(mod)), entrypath) (CSTParser-based module traverse), which means both ways work differently but there is no much difference between their performances.


As for goto, obviously CSTParser-based module traverse is better since we have to call toplevelitems anyway even if Revise-like file revealing succeeded. But in reality they are only called once for each module, so this won't be a big deal anyway.

aviatesk avatar Nov 14 '19 06:11 aviatesk