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

Refactor Symbol Cache #384

Merged
merged 2 commits into from
May 24, 2019
Merged

Refactor Symbol Cache #384

merged 2 commits into from
May 24, 2019

Conversation

Krzysztof-Cieslak
Copy link
Member

It turns out that sending thousands of symbols serialized to JSON through HTTP is not the best idea I've ever had!

I've started working on some performance analyses, and while I don't have yet lot of experience with it there was one big elephant in the room that was noticeable even for me:

The absurd amount of Int32 allocations caused by Newtonsoft.Json.

GC Heap Alloc Ignore Free:
image
image

Turns out this all was caused by single function - SymbolCache.sendSymbols that caused 56% of allocations, and was causing 12% of CPU usage:

GC Heap Alloc Ignore Free:
image

CPU Stack:
image

It was also causing ~40% of Large Object allocations:
GC Heap Alloc Ignore Free:
image


After the refactoring main FSAC process inserts symbols to SQLite directly, and only sends notification to the SymbolCache process that the DB was updated and it should be loaded to memory.

After the refactoring SymbolCache.sendSymbols causes 5% of CPU usage and 14% of allocations:
GC Heap Alloc Ignore Free:
image

CPU Stack:
image

I've also observed decrease on Large Object allocations from around 3% to 1% and GCStats are not showing any LOH allocation pauses.

Pre refactoring:
image

Post refactoring:
image

@vilinski
Copy link
Contributor

The absurd amount of Int32 allocations caused by Newtonsoft.Json.

How hard is it to replace it with something meaningful? Utf8Json is darn good on performance and allocations.

@Krzysztof-Cieslak
Copy link
Member Author

How hard is it to replace it with something meaningful?

Well, I removed the part of passing data with JSON completely.

res

let initializeCache dir =
let connectionString = sprintf "Data Source=%s/.ionide/symbolCache.db" dir
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we use /.ionide/obj , that will be git ignored by default by pratically all .gitignores, so less noise

Copy link
Contributor

@enricosada enricosada May 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is that it's passed as argument to FSAC.

So editors can put that:

  • or in their temp directory or custom dir
  • or can be in a dir inside %TEMP% generated for the workspace so valid until restart and automatically cleaned up.

do if not dbExists then
let fs = File.Create(dbPath)
fs.Close()
let cmd = "CREATE TABLE Symbols(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesnt work if the schema will be updated later to v2.
for example an older symbolcache exists on file, and that's will be used but schema is v1

@enricosada
Copy link
Contributor

@Krzysztof-Cieslak btw travis is red also in master, after bfa7f58 was green before

@Krzysztof-Cieslak
Copy link
Member Author

@enricosada, Regarding comments on code - I've only refactored code here for performance gain, the comments are relevant but I don't want to address them at the moment - it's just refactoring, I don't want to introduce any changes to the feature

@Krzysztof-Cieslak
Copy link
Member Author

This is ready to merge.

@TIHan
Copy link

TIHan commented May 24, 2019

What a big chungus. It's always great looking at your first run of perf view :)

@enricosada enricosada merged commit ec77eed into master May 24, 2019
@Krzysztof-Cieslak Krzysztof-Cieslak deleted the symolCacheRefactor branch June 7, 2019 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants