FAQ
Hello,

goimports is very fast with small GOPATH trees, but its performance scales
linearly with the number of files and directories in that tree. My GOPATH
has 15077 directories, 13250 of which contain "node_modules" in the name
and are part of a single 2 days prototype of a web application using
gulp/npm for the frontend. In those two days, the execution time of
goimports (in non-trivial cases) raised from ~0.1 seconds to ~3-4 seconds.
If I were working on multiple apps using that kind of technology, goimports
would become unusable.

The culprit is loadPkgIndex() in fix.go, which scans the whole GOPATH
building an index of the whole GOPATH (a map of package names -> full
path). This is invoked in all cases where there is an unresolved package
name in the source code which doesn't immediately map to the standard
library (there is a short-circuit for the standard library, so that
loadPkgIndex isn't called in that common case).

I think the main goal of goimports is to be bound to the save button of an
editor. If goimports takes several seconds to run, it's very annoying even
if it ends up producing the correct result. Given that it's only a
programming aid, I would rather have goimports always take 0.1 seconds, and
not be able to complete imports in some cases. IOW, a very fast goimports
beats a very correct goimports, IMO.

I brainstormed a few ideas on how to improve the speed of goimports, and I
would like to discuss them in the list before submitting a CL that goes in
a direction which is deemed completely wrong:

1) loadPkgIndex ignores all directories (and subtrees) with names starting
with a dot, an underscore, or whose name is "testdata". It might make sense
to grow this ignore list to cover common directory names pointing to trees
that are unlikely to be used as part of an import path. "node_modules" is
such an example, but also "bower_components" and "Godeps" come to mind. I
understand that growing such a hardcoded list is suboptimal, but it's a
quick stopgap with enormous effect on performances. Obviously it's just a
heuristic, and it might be wrong in some cases, but it would probably work
most of the times, and I think it would give an improvement to many people
using Go to program single-page web applications.

2) Similarly, we could avoid recursing more than N levels. This is just a
rule of thumb because it's highly unlikely that somebody is using an import
line with a path 10-15 components long. Most import paths are long between
3 and 5 components.

3) The index built by loadPkgIndex could be serialized to disk, and reused
for a certain period of time. I attempted a quick patch where the index is
serialized in a dot file in the home (through gob) and reused for 60
minutes, and it works very well for me. If I improved the patch to be smart
enough to always rebuild the part of the index that handles the current
project (that is, the current directory and its siblings), it would be
close to perfect.

4) goimports doesn't realize that an expression like "a.b" could refer to a
global unexported symbol "a" in the current package, so it always rebuilds
the index just in case there is a package named "a" that exports a symbol
called "b". A shortcut could be added so that if goimports realizes that
"a" is not a package name but just a global symbol in the current package,
it could remove it from the list of packages to be searched for; at that
point, the list would go down to zero in most normal cases, and thus the
frequency of calls to loadPkgIndex in a normal development loop would
drastically reduce.

Thanks in advance for your comments.

Giovanni

--
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Search Discussions

  • Andrew Gerrand at May 11, 2015 at 4:18 am

    On 11 May 2015 at 09:36, wrote:

    4) goimports doesn't realize that an expression like "a.b" could refer to
    a global unexported symbol "a" in the current package, so it always
    rebuilds the index just in case there is a package named "a" that exports a
    symbol called "b". A shortcut could be added so that if goimports realizes
    that "a" is not a package name but just a global symbol in the current
    package, it could remove it from the list of packages to be searched for;
    at that point, the list would go down to zero in most normal cases, and
    thus the frequency of calls to loadPkgIndex in a normal development loop
    would drastically reduce

    This is the most important step, IMO. It's the only time I find goimports
    to be unusably slow (I take it out of my on-save hook when working on such
    files). Otherwise it seems fast enough even though I have quite a large
    workspace.

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Fatih Arslan at May 11, 2015 at 3:28 pm
    Here is a previous conversation which is also about the slowness in
    goimports and some suggestions to improve it:
    https://groups.google.com/forum/#!topic/golang-dev/8p1zz_6Q7y0
    On Mon, May 11, 2015 at 2:36 AM, wrote:
    Hello,

    goimports is very fast with small GOPATH trees, but its performance scales
    linearly with the number of files and directories in that tree. My GOPATH
    has 15077 directories, 13250 of which contain "node_modules" in the name and
    are part of a single 2 days prototype of a web application using gulp/npm
    for the frontend. In those two days, the execution time of goimports (in
    non-trivial cases) raised from ~0.1 seconds to ~3-4 seconds. If I were
    working on multiple apps using that kind of technology, goimports would
    become unusable.

    The culprit is loadPkgIndex() in fix.go, which scans the whole GOPATH
    building an index of the whole GOPATH (a map of package names -> full path).
    This is invoked in all cases where there is an unresolved package name in
    the source code which doesn't immediately map to the standard library (there
    is a short-circuit for the standard library, so that loadPkgIndex isn't
    called in that common case).

    I think the main goal of goimports is to be bound to the save button of an
    editor. If goimports takes several seconds to run, it's very annoying even
    if it ends up producing the correct result. Given that it's only a
    programming aid, I would rather have goimports always take 0.1 seconds, and
    not be able to complete imports in some cases. IOW, a very fast goimports
    beats a very correct goimports, IMO.

    I brainstormed a few ideas on how to improve the speed of goimports, and I
    would like to discuss them in the list before submitting a CL that goes in a
    direction which is deemed completely wrong:

    1) loadPkgIndex ignores all directories (and subtrees) with names starting
    with a dot, an underscore, or whose name is "testdata". It might make sense
    to grow this ignore list to cover common directory names pointing to trees
    that are unlikely to be used as part of an import path. "node_modules" is
    such an example, but also "bower_components" and "Godeps" come to mind. I
    understand that growing such a hardcoded list is suboptimal, but it's a
    quick stopgap with enormous effect on performances. Obviously it's just a
    heuristic, and it might be wrong in some cases, but it would probably work
    most of the times, and I think it would give an improvement to many people
    using Go to program single-page web applications.

    2) Similarly, we could avoid recursing more than N levels. This is just a
    rule of thumb because it's highly unlikely that somebody is using an import
    line with a path 10-15 components long. Most import paths are long between 3
    and 5 components.

    3) The index built by loadPkgIndex could be serialized to disk, and reused
    for a certain period of time. I attempted a quick patch where the index is
    serialized in a dot file in the home (through gob) and reused for 60
    minutes, and it works very well for me. If I improved the patch to be smart
    enough to always rebuild the part of the index that handles the current
    project (that is, the current directory and its siblings), it would be close
    to perfect.

    4) goimports doesn't realize that an expression like "a.b" could refer to a
    global unexported symbol "a" in the current package, so it always rebuilds
    the index just in case there is a package named "a" that exports a symbol
    called "b". A shortcut could be added so that if goimports realizes that "a"
    is not a package name but just a global symbol in the current package, it
    could remove it from the list of packages to be searched for; at that point,
    the list would go down to zero in most normal cases, and thus the frequency
    of calls to loadPkgIndex in a normal development loop would drastically
    reduce.

    Thanks in advance for your comments.

    Giovanni

    --
    You received this message because you are subscribed to the Google Groups
    "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an
    email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.


    --
    Fatih Arslan

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Giovanni Bajo at May 11, 2015 at 2:38 pm
    Thanks. It looks like it's a long discussion on one of the four options
    that I proposed (option #3 in my email), and it looks like the discussion
    went into a direction that was deemed too big for the original proposer
    (that is, implementing an external indexing daemon). As a result, it's not
    clear to me whether somebody is working on anything.
    On Mon, May 11, 2015 at 1:25 PM, Fatih Arslan wrote:

    Here is a previous conversation which is also about the slowness in
    goimports and some suggestions to improve it:
    https://groups.google.com/forum/#!topic/golang-dev/8p1zz_6Q7y0
    On Mon, May 11, 2015 at 2:36 AM, wrote:
    Hello,

    goimports is very fast with small GOPATH trees, but its performance scales
    linearly with the number of files and directories in that tree. My GOPATH
    has 15077 directories, 13250 of which contain "node_modules" in the name and
    are part of a single 2 days prototype of a web application using gulp/npm
    for the frontend. In those two days, the execution time of goimports (in
    non-trivial cases) raised from ~0.1 seconds to ~3-4 seconds. If I were
    working on multiple apps using that kind of technology, goimports would
    become unusable.

    The culprit is loadPkgIndex() in fix.go, which scans the whole GOPATH
    building an index of the whole GOPATH (a map of package names -> full path).
    This is invoked in all cases where there is an unresolved package name in
    the source code which doesn't immediately map to the standard library (there
    is a short-circuit for the standard library, so that loadPkgIndex isn't
    called in that common case).

    I think the main goal of goimports is to be bound to the save button of an
    editor. If goimports takes several seconds to run, it's very annoying even
    if it ends up producing the correct result. Given that it's only a
    programming aid, I would rather have goimports always take 0.1 seconds, and
    not be able to complete imports in some cases. IOW, a very fast goimports
    beats a very correct goimports, IMO.

    I brainstormed a few ideas on how to improve the speed of goimports, and I
    would like to discuss them in the list before submitting a CL that goes in a
    direction which is deemed completely wrong:

    1) loadPkgIndex ignores all directories (and subtrees) with names starting
    with a dot, an underscore, or whose name is "testdata". It might make sense
    to grow this ignore list to cover common directory names pointing to trees
    that are unlikely to be used as part of an import path. "node_modules" is
    such an example, but also "bower_components" and "Godeps" come to mind. I
    understand that growing such a hardcoded list is suboptimal, but it's a
    quick stopgap with enormous effect on performances. Obviously it's just a
    heuristic, and it might be wrong in some cases, but it would probably work
    most of the times, and I think it would give an improvement to many people
    using Go to program single-page web applications.

    2) Similarly, we could avoid recursing more than N levels. This is just a
    rule of thumb because it's highly unlikely that somebody is using an import
    line with a path 10-15 components long. Most import paths are long between 3
    and 5 components.

    3) The index built by loadPkgIndex could be serialized to disk, and reused
    for a certain period of time. I attempted a quick patch where the index is
    serialized in a dot file in the home (through gob) and reused for 60
    minutes, and it works very well for me. If I improved the patch to be smart
    enough to always rebuild the part of the index that handles the current
    project (that is, the current directory and its siblings), it would be close
    to perfect.

    4) goimports doesn't realize that an expression like "a.b" could refer to a
    global unexported symbol "a" in the current package, so it always rebuilds
    the index just in case there is a package named "a" that exports a symbol
    called "b". A shortcut could be added so that if goimports realizes that "a"
    is not a package name but just a global symbol in the current package, it
    could remove it from the list of packages to be searched for; at that point,
    the list would go down to zero in most normal cases, and thus the frequency
    of calls to loadPkgIndex in a normal development loop would drastically
    reduce.

    Thanks in advance for your comments.

    Giovanni

    --
    You received this message because you are subscribed to the Google Groups
    "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an
    email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.


    --
    Fatih Arslan


    --
    Giovanni Bajo

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Josh Bleecher Snyder at May 11, 2015 at 5:22 pm

    Thanks. It looks like it's a long discussion on one of the four options that
    I proposed (option #3 in my email), and it looks like the discussion went
    into a direction that was deemed too big for the original proposer (that is,
    implementing an external indexing daemon). As a result, it's not clear to me
    whether somebody is working on anything.
    I don't think anyone is working on anything. I'm sure they'll speak up
    here soon if they are.

    I agree with Andrew; option #4 seems like a great place to start. If
    you mail CL implementing that, please cc me on it.

    -josh

    On Mon, May 11, 2015 at 1:25 PM, Fatih Arslan wrote:

    Here is a previous conversation which is also about the slowness in
    goimports and some suggestions to improve it:
    https://groups.google.com/forum/#!topic/golang-dev/8p1zz_6Q7y0
    On Mon, May 11, 2015 at 2:36 AM, wrote:
    Hello,

    goimports is very fast with small GOPATH trees, but its performance
    scales
    linearly with the number of files and directories in that tree. My
    GOPATH
    has 15077 directories, 13250 of which contain "node_modules" in the name
    and
    are part of a single 2 days prototype of a web application using
    gulp/npm
    for the frontend. In those two days, the execution time of goimports (in
    non-trivial cases) raised from ~0.1 seconds to ~3-4 seconds. If I were
    working on multiple apps using that kind of technology, goimports would
    become unusable.

    The culprit is loadPkgIndex() in fix.go, which scans the whole GOPATH
    building an index of the whole GOPATH (a map of package names -> full
    path).
    This is invoked in all cases where there is an unresolved package name
    in
    the source code which doesn't immediately map to the standard library
    (there
    is a short-circuit for the standard library, so that loadPkgIndex isn't
    called in that common case).

    I think the main goal of goimports is to be bound to the save button of
    an
    editor. If goimports takes several seconds to run, it's very annoying
    even
    if it ends up producing the correct result. Given that it's only a
    programming aid, I would rather have goimports always take 0.1 seconds,
    and
    not be able to complete imports in some cases. IOW, a very fast
    goimports
    beats a very correct goimports, IMO.

    I brainstormed a few ideas on how to improve the speed of goimports, and
    I
    would like to discuss them in the list before submitting a CL that goes
    in a
    direction which is deemed completely wrong:

    1) loadPkgIndex ignores all directories (and subtrees) with names
    starting
    with a dot, an underscore, or whose name is "testdata". It might make
    sense
    to grow this ignore list to cover common directory names pointing to
    trees
    that are unlikely to be used as part of an import path. "node_modules"
    is
    such an example, but also "bower_components" and "Godeps" come to mind.
    I
    understand that growing such a hardcoded list is suboptimal, but it's a
    quick stopgap with enormous effect on performances. Obviously it's just
    a
    heuristic, and it might be wrong in some cases, but it would probably
    work
    most of the times, and I think it would give an improvement to many
    people
    using Go to program single-page web applications.

    2) Similarly, we could avoid recursing more than N levels. This is just
    a
    rule of thumb because it's highly unlikely that somebody is using an
    import
    line with a path 10-15 components long. Most import paths are long
    between 3
    and 5 components.

    3) The index built by loadPkgIndex could be serialized to disk, and
    reused
    for a certain period of time. I attempted a quick patch where the index
    is
    serialized in a dot file in the home (through gob) and reused for 60
    minutes, and it works very well for me. If I improved the patch to be
    smart
    enough to always rebuild the part of the index that handles the current
    project (that is, the current directory and its siblings), it would be
    close
    to perfect.

    4) goimports doesn't realize that an expression like "a.b" could refer
    to a
    global unexported symbol "a" in the current package, so it always
    rebuilds
    the index just in case there is a package named "a" that exports a
    symbol
    called "b". A shortcut could be added so that if goimports realizes that
    "a"
    is not a package name but just a global symbol in the current package,
    it
    could remove it from the list of packages to be searched for; at that
    point,
    the list would go down to zero in most normal cases, and thus the
    frequency
    of calls to loadPkgIndex in a normal development loop would drastically
    reduce.

    Thanks in advance for your comments.

    Giovanni

    --
    You received this message because you are subscribed to the Google
    Groups
    "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send
    an
    email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.


    --
    Fatih Arslan



    --
    Giovanni Bajo

    --
    You received this message because you are subscribed to the Google Groups
    "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an
    email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Alberto García Hierro at May 14, 2015 at 2:11 pm
    I do have a "hackish" solution working, which uses the directory mtime to
    invalidate the cache. Works for me, but I do understand that it's an
    unreliable mechanism, so I don't have plans to even submit a CL.

    On the other hand, I don't think think fixing #4 is worth the effort. I've
    checked several editors and most of the time they feed goimports from stdin
    using the current buffer, so there's no way goimports can't find about
    identifiers in other files from the same package. One could argue that the
    editor/plugin authors might be doing it wrong by feeding goimports from
    stdin, but I have no idea about how difficult might be to change that.

    Regards,
    Alberto
    On Monday, May 11, 2015 at 6:22:33 PM UTC+1, Josh Bleecher Snyder wrote:

    Thanks. It looks like it's a long discussion on one of the four options that
    I proposed (option #3 in my email), and it looks like the discussion went
    into a direction that was deemed too big for the original proposer (that is,
    implementing an external indexing daemon). As a result, it's not clear to me
    whether somebody is working on anything.
    I don't think anyone is working on anything. I'm sure they'll speak up
    here soon if they are.

    I agree with Andrew; option #4 seems like a great place to start. If
    you mail CL implementing that, please cc me on it.

    -josh

    On Mon, May 11, 2015 at 1:25 PM, Fatih Arslan <ftha...@gmail.com
    <javascript:>> wrote:
    Here is a previous conversation which is also about the slowness in
    goimports and some suggestions to improve it:
    https://groups.google.com/forum/#!topic/golang-dev/8p1zz_6Q7y0

    On Mon, May 11, 2015 at 2:36 AM, <giovan...@gmail.com <javascript:>>
    wrote:
    Hello,

    goimports is very fast with small GOPATH trees, but its performance
    scales
    linearly with the number of files and directories in that tree. My
    GOPATH
    has 15077 directories, 13250 of which contain "node_modules" in the
    name
    and
    are part of a single 2 days prototype of a web application using
    gulp/npm
    for the frontend. In those two days, the execution time of goimports
    (in
    non-trivial cases) raised from ~0.1 seconds to ~3-4 seconds. If I
    were
    working on multiple apps using that kind of technology, goimports
    would
    become unusable.

    The culprit is loadPkgIndex() in fix.go, which scans the whole GOPATH
    building an index of the whole GOPATH (a map of package names -> full
    path).
    This is invoked in all cases where there is an unresolved package
    name
    in
    the source code which doesn't immediately map to the standard library
    (there
    is a short-circuit for the standard library, so that loadPkgIndex
    isn't
    called in that common case).

    I think the main goal of goimports is to be bound to the save button
    of
    an
    editor. If goimports takes several seconds to run, it's very annoying
    even
    if it ends up producing the correct result. Given that it's only a
    programming aid, I would rather have goimports always take 0.1
    seconds,
    and
    not be able to complete imports in some cases. IOW, a very fast
    goimports
    beats a very correct goimports, IMO.

    I brainstormed a few ideas on how to improve the speed of goimports,
    and
    I
    would like to discuss them in the list before submitting a CL that
    goes
    in a
    direction which is deemed completely wrong:

    1) loadPkgIndex ignores all directories (and subtrees) with names
    starting
    with a dot, an underscore, or whose name is "testdata". It might make
    sense
    to grow this ignore list to cover common directory names pointing to
    trees
    that are unlikely to be used as part of an import path.
    "node_modules"
    is
    such an example, but also "bower_components" and "Godeps" come to
    mind.
    I
    understand that growing such a hardcoded list is suboptimal, but it's
    a
    quick stopgap with enormous effect on performances. Obviously it's
    just
    a
    heuristic, and it might be wrong in some cases, but it would probably
    work
    most of the times, and I think it would give an improvement to many
    people
    using Go to program single-page web applications.

    2) Similarly, we could avoid recursing more than N levels. This is
    just
    a
    rule of thumb because it's highly unlikely that somebody is using an
    import
    line with a path 10-15 components long. Most import paths are long
    between 3
    and 5 components.

    3) The index built by loadPkgIndex could be serialized to disk, and
    reused
    for a certain period of time. I attempted a quick patch where the
    index
    is
    serialized in a dot file in the home (through gob) and reused for 60
    minutes, and it works very well for me. If I improved the patch to be
    smart
    enough to always rebuild the part of the index that handles the
    current
    project (that is, the current directory and its siblings), it would
    be
    close
    to perfect.

    4) goimports doesn't realize that an expression like "a.b" could
    refer
    to a
    global unexported symbol "a" in the current package, so it always
    rebuilds
    the index just in case there is a package named "a" that exports a
    symbol
    called "b". A shortcut could be added so that if goimports realizes
    that
    "a"
    is not a package name but just a global symbol in the current
    package,
    it
    could remove it from the list of packages to be searched for; at that
    point,
    the list would go down to zero in most normal cases, and thus the
    frequency
    of calls to loadPkgIndex in a normal development loop would
    drastically
    reduce.

    Thanks in advance for your comments.

    Giovanni

    --
    You received this message because you are subscribed to the Google
    Groups
    "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it,
    send
    an
    email to golang-dev+...@googlegroups.com <javascript:>.
    For more options, visit https://groups.google.com/d/optout.


    --
    Fatih Arslan



    --
    Giovanni Bajo

    --
    You received this message because you are subscribed to the Google Groups
    "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an
    email to golang-dev+...@googlegroups.com <javascript:>.
    For more options, visit https://groups.google.com/d/optout.
    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Roger peppe at May 14, 2015 at 2:53 pm

    On 14 May 2015 at 15:11, Alberto García Hierro wrote:
    I do have a "hackish" solution working, which uses the directory mtime to
    invalidate the cache. Works for me, but I do understand that it's an
    unreliable mechanism, so I don't have plans to even submit a CL.

    On the other hand, I don't think think fixing #4 is worth the effort. I've
    checked several editors and most of the time they feed goimports from stdin
    using the current buffer, so there's no way goimports can't find about
    identifiers in other files from the same package. One could argue that the
    editor/plugin authors might be doing it wrong by feeding goimports from
    stdin, but I have no idea about how difficult might be to change that.
    There's actually not much technical difficulty in doing that.
    godef reads input from stdin, but also finds definitions in the
    current directory.

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Alberto García Hierro at May 14, 2015 at 6:44 pm
    That seems like it could cause subtle problems. What happens if I'm editing
    foo/bar/baz.go? The tool will read the definitions from package foo (since
    it's the working directory where I invoked $EDITOR) while it's actually
    feeding it a file from package bar.
    On Thursday, May 14, 2015 at 3:53:09 PM UTC+1, rog wrote:

    On 14 May 2015 at 15:11, Alberto García Hierro <alb...@garciahierro.com
    <javascript:>> wrote:
    I do have a "hackish" solution working, which uses the directory mtime to
    invalidate the cache. Works for me, but I do understand that it's an
    unreliable mechanism, so I don't have plans to even submit a CL.

    On the other hand, I don't think think fixing #4 is worth the effort. I've
    checked several editors and most of the time they feed goimports from stdin
    using the current buffer, so there's no way goimports can't find about
    identifiers in other files from the same package. One could argue that the
    editor/plugin authors might be doing it wrong by feeding goimports from
    stdin, but I have no idea about how difficult might be to change that.
    There's actually not much technical difficulty in doing that.
    godef reads input from stdin, but also finds definitions in the
    current directory.
    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Roger peppe at May 15, 2015 at 11:00 am

    On 14 May 2015 at 19:44, Alberto García Hierro wrote:
    That seems like it could cause subtle problems. What happens if I'm editing
    foo/bar/baz.go? The tool will read the definitions from package foo (since
    it's the working directory where I invoked $EDITOR) while it's actually
    feeding it a file from package bar.
    godef currently requires that you pass it the file name as well as
    providing standard input, from which it can infer the correct package
    to parse.

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Alberto García Hierro at May 15, 2015 at 12:15 pm
    That would work, but it would break a lot of editor plugins which are just
    passing the input using stdin without the filename. I don't think breaking
    goimports is worth it (specially, since it's intended as a gofmt
    replacement and the latter also works with just stdin, no filename), but
    that's not my call to make. Also, it might be desirable to keep
    tools/imports (where +90% of the execution time of goimports is spent) able
    to work only on memory, with no filesystem available.

    In any case, teaching goimports about other identifiers in the same package
    feels like a bandaid solution to me. It would be better to just make it
    fast at finding Go packages and the other issues would just go away (if you
    take finding Go pkgs out of the equation, goimports takes less than a few
    milliseconds to do its job).
    On Friday, May 15, 2015 at 12:00:48 PM UTC+1, rog wrote:

    On 14 May 2015 at 19:44, Alberto García Hierro <alb...@garciahierro.com
    <javascript:>> wrote:
    That seems like it could cause subtle problems. What happens if I'm editing
    foo/bar/baz.go? The tool will read the definitions from package foo (since
    it's the working directory where I invoked $EDITOR) while it's actually
    feeding it a file from package bar.
    godef currently requires that you pass it the file name as well as
    providing standard input, from which it can infer the correct package
    to parse.
    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Ian Davis at May 15, 2015 at 12:24 pm

    On Fri, May 15, 2015, at 01:15 PM, Alberto García Hierro wrote:
    That would work, but it would break a lot of editor plugins which are
    just passing the input using stdin without the filename. I don't think
    breaking goimports is worth it (specially, since it's intended as a
    gofmt replacement and the latter also works with just stdin, no
    filename), but that's not my call to make. Also, it might be desirable
    to keep tools/imports (where +90% of the execution time of goimports
    is spent) able to work only on memory, with no filesystem available.

    In any case, teaching goimports about other identifiers in the same
    package feels like a bandaid solution to me. It would be better to
    just make it fast at finding Go packages and the other issues would
    just go away (if you take finding Go pkgs out of the equation,
    goimports takes less than a few milliseconds to do its job).
    It's not a bandaid, it would address a real problem that makes goimports
    much less useful than it could be. Example: if you have a package level
    variable called log then goimports attempts to add "log" as an import.

    It means you can't have package level variables that might conflict with
    any package name in your GOPATH.

    Ian

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Alberto García Hierro at May 15, 2015 at 1:44 pm
    That would make goimports more correct, no doubt about it, but I think your
    example is a bit contrived. I've written a good amount of Go code and I
    have yet to find an instance of this problem.

    Regarding speed improvements, which seems to be what the majority of people
    want to achieve, it would do next to nothing.
    On Friday, May 15, 2015 at 1:24:13 PM UTC+1, Ian Davis wrote:


    On Fri, May 15, 2015, at 01:15 PM, Alberto García Hierro wrote:

    That would work, but it would break a lot of editor plugins which are just
    passing the input using stdin without the filename. I don't think breaking
    goimports is worth it (specially, since it's intended as a gofmt
    replacement and the latter also works with just stdin, no filename), but
    that's not my call to make. Also, it might be desirable to keep
    tools/imports (where +90% of the execution time of goimports is spent) able
    to work only on memory, with no filesystem available.

    In any case, teaching goimports about other identifiers in the same
    package feels like a bandaid solution to me. It would be better to just
    make it fast at finding Go packages and the other issues would just go away
    (if you take finding Go pkgs out of the equation, goimports takes less than
    a few milliseconds to do its job).


    It's not a bandaid, it would address a real problem that makes goimports
    much less useful than it could be. Example: if you have a package level
    variable called log then goimports attempts to add "log" as an import.

    It means you can't have package level variables that might conflict with
    any package name in your GOPATH.

    Ian
    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Josh Bleecher Snyder at May 15, 2015 at 2:54 pm

    That would make goimports more correct, no doubt about it, but I think your
    example is a bit contrived. I've written a good amount of Go code and I have
    yet to find an instance of this problem.
    Ian is not the only one: golang.org/issue/7463

    -josh


    On Friday, May 15, 2015 at 1:24:13 PM UTC+1, Ian Davis wrote:


    On Fri, May 15, 2015, at 01:15 PM, Alberto García Hierro wrote:

    That would work, but it would break a lot of editor plugins which are just
    passing the input using stdin without the filename. I don't think breaking
    goimports is worth it (specially, since it's intended as a gofmt replacement
    and the latter also works with just stdin, no filename), but that's not my
    call to make. Also, it might be desirable to keep tools/imports (where +90%
    of the execution time of goimports is spent) able to work only on memory,
    with no filesystem available.

    In any case, teaching goimports about other identifiers in the same
    package feels like a bandaid solution to me. It would be better to just make
    it fast at finding Go packages and the other issues would just go away (if
    you take finding Go pkgs out of the equation, goimports takes less than a
    few milliseconds to do its job).


    It's not a bandaid, it would address a real problem that makes goimports
    much less useful than it could be. Example: if you have a package level
    variable called log then goimports attempts to add "log" as an import.

    It means you can't have package level variables that might conflict with
    any package name in your GOPATH.

    Ian
    --
    You received this message because you are subscribed to the Google Groups
    "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an
    email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Alberto García Hierro at May 15, 2015 at 5:23 pm

    On Friday, May 15, 2015 at 3:54:41 PM UTC+1, Josh Bleecher Snyder wrote:
    That would make goimports more correct, no doubt about it, but I think your
    example is a bit contrived. I've written a good amount of Go code and I have
    yet to find an instance of this problem.
    Ian is not the only one: golang.org/issue/7463
    It seems to me that the comments in the issue agree with my opinion on the
    matter. As I'm sure you know, fixing that requires a non-trivial amount of
    work and it's very likely to make goimports slower (you'd probably need to
    use go/loader with all its drawbacks). Not worth the effort in my opinion.

    On the other hand, and just to keep moving on the direction of the original
    topic, the more I think of the idea of a "non-visible" daemon for the
    package index that you suggested and we talked about the other day (it was
    off-list IIRC), the more I like it. I don't have the time to work on that
    right now, but it seems like the best approach so far.

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Andrew Gerrand at May 18, 2015 at 2:40 am

    On 15 May 2015 at 23:44, Alberto García Hierro wrote:

    That would make goimports more correct, no doubt about it, but I think
    your example is a bit contrived. I've written a good amount of Go code and
    I have yet to find an instance of this problem.

    Regarding speed improvements, which seems to be what the majority of
    people want to achieve, it would do next to nothing.
    Yet the first reply to this thread was me calling it out as the only real
    problem I have with goimports:
    On 11 May 2015 at 14:18, Andrew Gerrand wrote:

    This is the most important step, IMO. It's the only time I find goimports
    to be unusably slow (I take it out of my on-save hook when working on such
    files). Otherwise it seems fast enough even though I have quite a large
    workspace.
    By all means make your point, but please also read the thread and
    acknowledge other people's input.

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Giovanni Bajo at May 15, 2015 at 12:47 pm

    On Fri, May 15, 2015 at 2:15 PM, Alberto García Hierro wrote:

    That would work, but it would break a lot of editor plugins which are just
    passing the input using stdin without the filename. I don't think breaking
    goimports is worth it (specially, since it's intended as a gofmt
    replacement and the latter also works with just stdin, no filename), but
    that's not my call to make.
    It can be an optional optimization. If the filename is available, goimports
    is able to find unexported symbols in the current package, and thus reduce
    the number of times it scans the GOPATH.

    Also, it might be desirable to keep tools/imports (where +90% of the
    execution time of goimports is spent) able to work only on memory, with no
    filesystem available.
    The proposed code would neither increase nor reduce the number of times the
    filesystem is touched; you can scan the current packages only when there is
    at least one unknown symbol in the source code, but before indexing the
    whole GOPATH.

    In any case, teaching goimports about other identifiers in the same
    package feels like a bandaid solution to me. It would be better to just
    make it fast at finding Go packages and the other issues would just go away
    (if you take finding Go pkgs out of the equation, goimports takes less than
    a few milliseconds to do its job).
    I agree that it's just a part of a more global solution, but I wouldn't
    call it bandaid. When you have a bottleneck in your software, both making
    the code faster and reducing the number of times it is invoked are valid
    optimizations.
    --
    Giovanni Bajo

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Giovanni Bajo at May 14, 2015 at 7:01 pm

    On Thu, May 14, 2015 at 4:11 PM, Alberto García Hierro wrote:
    On the other hand, I don't think think fixing #4 is worth the effort. I've
    checked several editors and most of the time they feed goimports from stdin
    using the current buffer, so there's no way goimports can't find about
    identifiers in other files from the same package. One could argue that the
    editor/plugin authors might be doing it wrong by feeding goimports from
    stdin, but I have no idea about how difficult might be to change that.
    Well, I think editors (plugins) will be fixed when people complain. I
    checked mine (Sublime Text 3), and the GoSublime plugin invokes goimport
    passing the full path name of the file, so at least in my case it would fix
    the issue.
    --
    Giovanni Bajo

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedMay 11, '15 at 3:30a
activeMay 18, '15 at 2:40a
posts17
users7
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase