[Instrument] Add method for loading multiple instruments data at once #6869
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
driusan
added
State: Blocked
|
Blocked by #6868 |
driusan
force-pushed
the
bulkLoadInstanceData
branch
from
c70d566 to
ed6eb24
Compare
laemtl
removed
the
State: Blocked
|
There is some minor linting errors to fix: FILE: ...nner/work/Loris/Loris/php/libraries/NDB_BVL_Instrument.class.incFOUND 2 ERRORS AFFECTING 2 LINES2142 | ERROR | [x] Equals sign not aligned with surrounding |
laemtl
approved these changes
Nov 23, 2020
laemtl
added
the
Passed manual tests
ridz1208
requested changes
Dec 2, 2020
driusan
added
the
State: Needs work
|
@driusan will you be fixing this one ? |
|
Eventually. |
NDB_BVL_Instrument has an.. unusual.. way of dealing with instrument data. Rather than having a class method for getting the data, there's a static method which takes an instrument as a parameter. The result is that data can not be cached in the object and needs to be continuously re-loaded from the database every time if it's needed multiple times in the same request. (For instance, calling getFieldValue for different fields in a script.) This adds a normal getter and caches the results in the object, deprecating the old static loadInstanceData and its inexplicable behaviour.
There is currently no mechanism to load the data from multiple
instrument instances at the same time efficiently and work with them
in PHP. (ie. to work the the data from multiple commentIDs for a single
instrument type in batch in a script) other than directly querying SQL.
However, this has other drawbacks such as the you only know whether the
instrument data should be loaded from the flag.Data column or a separate
table once the instrument is instantiated.
This adds a bulkLoadInstanceData function which will load the data from
multiple commentIDs at the same time in a single SQL query and return an
array of \NDB_BVL_Instruments with the data loaded.
In my non-scientific benchmarking where I loaded all the data from a single
instrument type on my sandbox multiple times, and printed the value of a single
field this (unsurprisingly) resulted in significant performance gains.
The naive way where factory is called multiple times took approximately
12 seconds:
```
foreach($commentIDs as $cid) {
$inst = \NDB_BVL_Instrument::factory('adi_r_proband', $cid);
print $inst->getFieldValue('informant_relation');
}
```
The slightly smarter way where we avoid the overhead of re-parsing the
instrument took approximately 1.2 seconds:
```
$inst = \NDB_BVL_Instrument::factory('adi_r_proband');
foreach($commentIDs as $cid) {
// note: must hack NDB_BVL_Instrument to make instanceData
// public for this to work.
$inst->instanceData = null;
$inst->commentID = $cid;
print $inst->getFieldValue('informant_relation');
}
```
The new function took approximately 0.2s:
```
$inst = \NDB_BVL_Instrument::factory('adi_r_proband');
$data = $inst->bulkLoadInstanceData($commentIDs);
foreach($data as $i) {
print $i->getFieldValue('informant_relation');
}
```
Co-authored-by: Rida Abou-Haidar <ridz1208@users.noreply.github.com>
driusan
force-pushed
the
bulkLoadInstanceData
branch
from
e0b387f to
300c8ae
Compare
|
I've rebased this and fixed the issue in the long thread. I didn't retest this, whoever tests this should ensure that when bulkLoadInstanceData is called on a CommentID that has a null Data column for a JSON instrument, calling |
|
@laemtl I just saw that you tested/approved this in November.. can you re-test the case in my comment above since the behaviour has changed? |
driusan
removed
the
State: Needs work
ridz1208
approved these changes
Mar 17, 2021
ridz1208
removed
the
Passed manual tests
|
@driusan I get an SQL error if $commentIDs is empty: Same with null (this case may be fine) They are corner cases but it may worth considering returning null if such value is passed. |
laemtl
added
the
Passed manual tests
|
@driusan Everything else works perfectly. Approving. |
AlexandraLivadas
pushed a commit
to AlexandraLivadas/Loris
that referenced
this pull request
Jun 29, 2021
…aces#6869) There is currently no mechanism to load the data from multiple instrument instances at the same time efficiently and work with them in PHP. (ie. to work the the data from multiple commentIDs for a single instrument type in batch in a script) other than directly querying SQL. However, this has other drawbacks such as the you only know whether the instrument data should be loaded from the flag.Data column or a separate table once the instrument is instantiated. This adds a bulkLoadInstanceData function which will load the data from multiple commentIDs at the same time in a single SQL query and return an array of \NDB_BVL_Instruments with the data loaded. In my non-scientific benchmarking where I loaded all the data from a single instrument type on my sandbox multiple times, and printed the value of a single field this (unsurprisingly) resulted in significant performance gains. The naive way where factory is called multiple times took approximately 12 seconds: ``` foreach($commentIDs as $cid) { $inst = \NDB_BVL_Instrument::factory('adi_r_proband', $cid); print $inst->getFieldValue('informant_relation'); } ``` The slightly smarter way where we avoid the overhead of re-parsing the instrument took approximately 1.2 seconds: ``` $inst = \NDB_BVL_Instrument::factory('adi_r_proband'); foreach($commentIDs as $cid) { // note: must hack NDB_BVL_Instrument to make instanceData // public for this to work. $inst->instanceData = null; $inst->commentID = $cid; print $inst->getFieldValue('informant_relation'); } ``` The new function took approximately 0.2s: ``` $inst = \NDB_BVL_Instrument::factory('adi_r_proband'); $data = $inst->bulkLoadInstanceData($commentIDs); foreach($data as $i) { print $i->getFieldValue('informant_relation'); } ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There is currently no mechanism to load the data from multiple
instrument instances at the same time efficiently and work with them
in PHP. (ie. to work the the data from multiple commentIDs for a single
instrument type in batch in a script) other than directly querying SQL.
However, this has other drawbacks such as the fact that you only know
whether the instrument data should be loaded from the flag.Data column
or a separate table once the instrument is instantiated.
This adds a bulkLoadInstanceData function which will load the data from
multiple commentIDs at the same time in a single SQL query and return an
array of \NDB_BVL_Instruments with the data loaded.
In my non-scientific benchmarking where I loaded all the data from a single
instrument type on my sandbox multiple times, and printed the value of a single
field this (unsurprisingly) resulted in significant performance gains.
The naive way where factory is called multiple times took approximately
12 seconds:
The slightly smarter way where we avoid the overhead of re-parsing the
instrument took approximately 1.2 seconds:
The new function took approximately 0.2s: