BigBahss / vscode-cmantic

C/C++ code generation for VS Code: Generate Definitions, Getters, Setters, and much more.
https://bigbahss.github.io/vscode-cmantic/
MIT License
83 stars 9 forks source link

Improving the locality of Switch Source/Header. #13

Closed ShadyAbuKalam closed 3 years ago

ShadyAbuKalam commented 3 years ago

Most of C++ projects either have the Cpp/header files in the same directory or in a separate src/inc folders in the same parent directory.

I suggest altering the searching techniques to first the parent directory, then the whole workspace folders if not found. This will boost the search especially in remotely edited workspaces. ( Our projects are on linux servers in HQ, while we access remotely via VPN - due to COVID).

BigBahss commented 3 years ago

That's a great idea, thank you for the suggestion. I had not considered that finding header/source pairs would be noticeably slow for complex/remote workspaces. I will implement this.

BigBahss commented 3 years ago

I'm curious, does switching header/source work faster for subsequent switches? I'm asking because header/source pairs are cached after they are found, and I want to make sure that's working correctly.

ShadyAbuKalam commented 3 years ago

I have went through your code and I have already seen the caching, and it's a YES.

The only down side to this, if the cache is not invalidated due to reasons outside VS code. I have seen that you utilize onDidDeleteFiles, but this doesn't watch the file on the harddisk. I know it's rare to occur, but I can think of changing branches of version control system using CLI or any other VCS ui tool. I believe the solution is simply to check if the target file is already there.

BigBahss commented 3 years ago

Yeah, I realized that potential problem when I implemented those event listeners. When I initially implemented the cache I was checking to see if the file exists, but my reasoning for switching to the listeners was to make cache retrieval as fast as possible. I think I can implement file system watcher that will detect changes outside of VS Code.

ShadyAbuKalam commented 3 years ago

Mmm, I don't think the check will be a real performance killer. I measured file checks in bash for 1000 times on the following machines Linux Dell server ( Intel Xeon - SSD harddisk ) real 0m0.015s user 0m0.08s sys 0m0.07s

Dell precision 7720 ( HDD ): real 0m0.027s user 0m0.015s sys 0m0.000s


I am not sure about how Electron implements this, but I don't think it would be noticeable difference for a single check.

Considering file system watcher, It's a bit annoying to have cross-platform one functioning 100% correct. So If you are gonna use it only for this feature, I don't think it would deserve the exerted effort.

BigBahss commented 3 years ago

Thanks for the benchmarks, I'll just revert back to checking that the file still exists.

BigBahss commented 3 years ago

Fixed in v0.5.0

ShadyAbuKalam commented 3 years ago

I've been trying this feature for the past half hour. It gave a good boost, I can't deny it. But it's still if the parent directory is kinda huge, the following two suggestion may help it. 1- Search for the pair in the same folder. 2- Allow configuration to specify the relative directories to search in. Few projects have src/inc folder pairs, but at the same level also have assets files - which sometimes may be very large amount. default values could be the following list

{
"header_folders" : ["header","inc","include"],
"source_folders" : ["source","src"]
}
BigBahss commented 3 years ago

I completely missed your update comment, so sorry about that 😔. I think this is a good idea, I will implement this.

BigBahss commented 3 years ago

I uploaded a pre-release here: https://github.com/BigBahss/vscode-cmantic/releases/tag/v0.6.3-alpha.1 Added settings C_mantic.folders.headerFiles and C_mantic.folders.sourceFiles to specify glob patterns to use for each. I decided not to use default values since I didn't want to break functionality in-case a user doesn't have directories matching the defaults (by default, the whole workspace is searched). If you could test that out and let me know if that improves things that would be great.

ShadyAbuKalam commented 3 years ago

VS Code states it can't find the C-mantic switch command anymore !!

BigBahss commented 3 years ago

That's very weird... I'll try to figure out why that is.

BigBahss commented 3 years ago

I tried the package on Linux and Windows and they both work... maybe try uninstalling it and reinstalling it if you haven't done so already, and if that doesn't work can you check the Output panel and see if the C-mantic channel shows up, and if so check whether it says that the extension was activated.

ShadyAbuKalam commented 3 years ago

And in the output console, the extension is activated.

[22:47:10 Info] C-mantic extension activated.

Looks like It doesn't find matched pair. Here is my configuration

{
    "C_mantic.folders.headerFiles": [".","../inc","../include"],
    "C_mantic.folders.sourceFiles": [".","../src","../source"],

}
BigBahss commented 3 years ago

The patterns need to be glob patterns (more about that here), C-mantic searches the workspace using these patterns, they can't relative file paths. Basically you just need to list the names of the directories, or if you want to search recursively in those folders, for example use inc/**.

BigBahss commented 3 years ago

Also, if you give the full workspace relative path, that will also most likely give better performance. For example, if you have these folders "app/foo/bar/src" in your workspace, then you could list that full path (relative to the workspace).

BigBahss commented 3 years ago

Also also, the command is simply Switch Header/Source in Workspace, it doesn't begin with 'C-mantic', simply because I didnt want it to clutter the editor context menu, since it uses the name of the command.

ShadyAbuKalam commented 3 years ago

I have downloaded your code and been looking into it. I see you use relative globs to the workspace root folder. So to find a pair cpp/h, vs code will search the whole workspace for path like

"**/inc/MyTargetFile.{h,hpp}"

This is not improvement at all. I truly suggest going into relative directories.

Another thing, it's a great idea to search for the pair upon opening a file. But why you delete it from the cache onClosing, you already check for pair existence on switching.

Please check the pull-request, it may be helpful for you. N.B: I don't write TypeScript at all.

BigBahss commented 3 years ago

As I said, you could have specified the workspace relative path, which would definitely have been an improvement. I don't think it's a good idea to require the user to specify relative paths between headers and source files, and I have also decided against having user specified glob patterns as well. This doesn't seem to be a problem that anyone else is experiencing, and since I don't have access to a remote workspace, this is difficult for me to test. I'm struggling to imagine how a glob search could be noticeably slow on a remote workspace, assuming that everything else works fine. It just seems like if a glob search is slow, then everything should be slow.

I have changed the code to first search the current directory which will be an improvement if the file exists there. I have also removed the event listener that removes the file from the cache when it is closed. I added that because I didn't want the cache to grow endlessly, and because in all my testing, finding the matching header/source when a file is opened happens very quickly. But, I removed it because honestly I don't think it matters either way.

Edit: Additionally, the search for matching header/source files will exclude searching directories that are specified in the Files: Exclude and Search: Exclude settings in vscode, which will also improve performance. So, if you have large directories with assets that you don't need to search in vscode, add those to the Search: Exclude setting.

BigBahss commented 3 years ago

I'm wondering what your thoughts are and whether or not you see an improvement by utilizing the Files: Exclude and Search: Exclude settings.

BigBahss commented 3 years ago

I will be closing this issue and the related PR since I haven't heard back from you, and because I haven't heard from anyone else who is having this problem.

ShadyAbuKalam commented 3 years ago

@BigBahss , for sure you can close it. I think the issue is due to the latency of VPN. It's not easily solvable.

I appreciate your efforts. I tried your plugin on local tree and it's very useful.

Cheers