Refactor the generate_files and to_file functions

[squirrelo]

The functions are horribly inefficient (By not sorting the dataframe and instead sorting in the SQL alone I was able to drop from 30ms to 10ms runtime) so these functions really need to be revisited. Also, once they are efficient we will no longer need to store the files, and instead can generate them on the fly removing the possibility of the files being out of sync with the database.

[adamrp]

Poking around in the code, it looks like TRN is used in a lot of places. Since generate_files and to_file functions are not going to modify the database, most of these seem unnecessary (e.g., here and here). I doubt this is driving the cost of these functions up very much, but it's at least something.

As an aside, 30ms (or 10ms) doesn't seem that bad, since we're not running these functions thousands of times a day or anything. Is this really something that we need to prioritize optimizing? It seems to me that we could already generate them on the fly even with the the current code, no?

[squirrelo]

This was on our test database data, but w have larger studies where the increase could be even more significant. Also, with so small a change making such a large impact, it's better to look through and see if we can't get any more speed out of the function. We could probably get an order of magnitude or more with tweaks like that one and using pandas + SQL more intelligently.

[josenavas]

@adamrp those calls to TRN are needed to make sure that the entire function is executed as a transaction

[adamrp]

Right -- but they are read-only, right? So is the concern that the data could be modified by a different query/process halfway through the execution of the function?

[wasade]

Is this actually a performance burden that is impacting users? What is the
real world impact of this method? I think what @adamrp is getting at is the
potential to accidentally optimize something that is only a fraction of
runtime, in which case, multiple orders of magnitude wouldn't result in a
practical benefit. The downsides are increase code entropy and complexity.
If this is really is impacting people, then 👍, otherwise, better to
leave be and focus elsewhere.

On Mon, Aug 24, 2015 at 6:41 PM, Joshua Shorenstein <
notifications@github.com> wrote:

This was on our test database data, but w have larger studies where the
increase could be even more significant. Also, with so small a change
making such a large impact, it's better to look through and see if we can't
get any more speed out of the function. We could probably get an order of
magnitude or more with tweaks like that one and using pandas + SQL more
intelligently.

—
Reply to this email directly or view it on GitHub
#1424 (comment).

[josenavas]

Yes, is to avoid data modification through the execution of the function.

The performance discussion comes by making the users generate the files every time that they want to download, rather than just storing the files as we are doing right now...

[wasade]

@josenavas, not sure I understand your comment. Are you suggesting there is a performance issue that is impacting people, or that there isn't? Or is this in reference to caching data for distribution?

[josenavas]

I think this is NOT a performance issue.

[wasade]

Okay, thanks

[squirrelo]

To clarify: We should be making the files on the fly instead of storing them, since that removes the possibility of the files being out of sync with the database, as said in the issue. There are also performance increases we can do to the file generation code that will facilitate this change, since it will be being called much more frequently when things are made on the fly.

[josenavas]

This requires some refactor that is not priority right now. There are other parts of the code that need to be refactored first.