search

howeyc / ledger

Command line double-entry accounting program
https://howeyc.github.io/ledger/
ISC License
461 stars 43 forks source link

Support sync access by default #18

Closed bionoren closed 5 years ago

bionoren commented 5 years ago

ParseLedger() is just a wrapper for ParseLedgerAsync, which means there is no public synchronous parsing API. If my program is going to be blocked anyway waiting for the results of a ledger parse, I don't want to have the overhead of channels and scheduler context switches on top of that.

Could we switch it around - so ParseLedger is where the work actually happens, and ParseLedgerAsync wraps it in a goroutine and channels? I would be happy to submit a pull request

bionoren commented 5 years ago

Alternatively, if you're concerned about parsing large ledger files, could we switch to a callback model as recommend by David Cheney? https://dave.cheney.net/practical-go/presentations/qcon-china.html#_leave_concurrency_to_the_caller

howeyc commented 5 years ago

I don't understand why the internal methods used within the package would matter to the caller.

If you want to handle the ledger as a stream of transactions, processing them as they are parsed, call ParseLedgerAsync(). If you want to call a synchronous, blocking function that returns all transactions as a single array, call ParseLedger().

If you're advocating for a public API that allows for the caller to pass a callback function that is executed per transaction, I'm going to respectfully decline.

bionoren commented 5 years ago

I realize I can call ParseLedger as a synchronous blocking API, my problem is that internally it’s asynchronous, and the memory allocations for those channels together with the context switching running a separate go-routine are a performance bottleneck for my application. I need a way to opt out of the goroutine and channels.

For context, benchmarking my application, ParseLedger takes 12% of total execution time (for a 15ms process). It’s not because ledger is inherently slow - I’ve looked at your benchmarks - it’s extra time in the scheduler and garbage collector due to, for me, unnecessary concurrency

howeyc commented 5 years ago

Interesting. Okay sure, go for it.

howeyc commented 5 years ago

LGTM