[ 
https://issues.apache.org/jira/browse/SSHD-1220?focusedWorklogId=672341&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-672341
 ]

ASF GitHub Bot logged work on SSHD-1220:
----------------------------------------

                Author: ASF GitHub Bot
            Created on: 30/Oct/21 12:05
            Start Date: 30/Oct/21 12:05
    Worklog Time Spent: 10m 
      Work Description: tomaswolf commented on pull request #207:
URL: https://github.com/apache/mina-sshd/pull/207#issuecomment-955198554


   > The code is certainly more complex to follow... It seems OK, but I may 
have missed something. Perhaps you should ask Guillaume to take a look at it as 
well - maybe he can locate something we both missed. In view of this I 
definitely recommend we make this a configurable behavior (default=enabled)
   
   Well, I think you know how I feel about this... Anyway: added such a config. 
But IMO it only adds unnecessary complication to the code, and users should 
never set it to false anyway. It really hurts performance. Try the directory 
listing test with it set to true or false:
   
   - true: takes some 70ms per run on my machine, makes 3 LSTAT calls
   - false: takes >2s per run on my machine, makes 10040 LSTAT calls
   
   The caching of attributes is truly _safe_: the unconditionally cached 
attributes are used _only_ in directory iteration; just like the cached 
attributes from `sun.nio.fs.BasicFileAttributesHolder` are used exclusively in 
`FileTreeWalker` in the Java library. And `withAttributeCaching` is used only 
for short code blocks that are _known_ to only make repeated calls to 
`readAttributes`, and they clear the cache at the end.
   
   My proposition instead of a pointless config: either we believe this 
implementation is correct and then check it in without config, or we believe it 
isn't correct and then we abandon this idea altogether.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


Issue Time Tracking
-------------------

    Worklog Id:     (was: 672341)
    Time Spent: 1.5h  (was: 1h 20m)

> SFTP: too many LSTAT calls
> --------------------------
>
>                 Key: SSHD-1220
>                 URL: https://issues.apache.org/jira/browse/SSHD-1220
>             Project: MINA SSHD
>          Issue Type: Improvement
>    Affects Versions: 2.7.0
>            Reporter: Thomas Wolf
>            Assignee: Thomas Wolf
>            Priority: Major
>          Time Spent: 1.5h
>  Remaining Estimate: 0h
>
> While looking at an alternate solution for SSHD-1217 I noticed that the 
> {{AbstractSftpSubsystemHelper}} makes several LSTAT calls for a single 
> {{FileSystem.readAttributes()}} invocation.
> Basically it makes one LSTAT call per supported attributes view; only to 
> collect the returned items then in one single map anyway.
> This doesn't make any sense. SFTP file attributes are a single, defined 
> structure. If {{AbstractSftpSubsystemHelper.getAttributes()}} needs to 
> collect all these different views ({{BasicFileAttributes}}, 
> {{PosixFileAttributes}}, and so on) on these underlying SFTP attributes, then 
> all these views should build upon the same single {{SftpClient.Attributes}} 
> gotten exactly _once_.
> This could be solved with careful temporary caching of SFTP attributes on the 
> {{SftpPath}} once read while in 
> {{AbstractSftpSubsystemHelper.getAttributes()}} and clearing that cache at 
> the end.
> The problem is more general, though. The code in several places makes 
> up-front checks whether a file exists, is a directory, and so on. This is a 
> questionable pattern anyway, since the result of a {{Files.exists()}} is 
> outdated immediately; one cannot rely on the file being still there on the 
> next access. With an {{SftpPath}}, this {{exists()}} call is an (L)STAT 
> remote call getting the attributes. Now look at 
> {{AbstractSftpSubsystemHelper.resolveFileAttributes}}: here getting the 
> attributes themselves is so guarded, so it makes at least _two_ LSTAT calls.
> This should also be improved.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org

Reply via email to