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.

Sign up for GitHub

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

Adds optimized bulk operations to DataView #572

Merged
merged 14 commits into from
Feb 2, 2021
Merged

Adds optimized bulk operations to DataView #572

Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
fbfc8e5
Add simple bulk update operations
kusc-leica Jan 21, 2021
4032750
Added bulk suspend option
kusc-leica Jan 21, 2021
20394a2
Use Map polyfill for better performance
kusc-leica Jan 21, 2021
1a4b82b
Ensure we clear the delete id lookup
kusc-leica Jan 21, 2021
04f3d12
Fix issue on Map polyfill migration
kusc-leica Jan 21, 2021
af956f0
Tests WIP
kusc-leica Jan 21, 2021
423ca67
Fixes and tests for new add operations
kusc-leica Jan 21, 2021
1781af8
Tests for bulk operations
kusc-leica Jan 21, 2021
fe6572a
Example added
kusc-leica Jan 21, 2021
311c897
Some additions to make performance difference more visible.
kusc-leica Jan 21, 2021
239995f
Moved map and added some docs+comments
kusc-leica Jan 21, 2021
94871cb
Added cypress test for performance
kusc-leica Jan 21, 2021
00438a2
Merge branch 'master' into feat/bulk-updates
kusc-leica Jan 25, 2021
171e48c
Moved exports of all files consistently to the bottom
kusc-leica Jan 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
184 changes: 152 additions & 32 deletions slick.dataview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,25 @@
}
});

/**
* Polyfill for Map to support old browsers but
* benefit of the Map speed in modern browsers.
*/
var SlickMap = 'Map' in window ? Map : function SlickMap() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice polyfill, I think you move this code into the slick.core.js so that it be used by other files, the core has these kind of util classes/fns so I think it's better there... and also expose it like the others and then you would call it with the dot instead new Slick.Map() (you can copy the structure of Slick.Event or the other ones in there)

The rest of the code looks quite clean, maybe just 1 other thing I could say is perhaps adding comments to some of the functions to explain what they do might be beneficial for future refactoring or understanding. I know the rest of the file is not well document but that doesn't mean we can't improve it, right? ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like you're faster with reviewing than I am with pinging you in the issue 😆

I think you move this code into the slick.core.js so that it be used by other files,

Sure, will do that. Adapting other areas will likely be beyond the scope of this PR.

1 other thing I could say is perhaps adding comments to some of the functions to explain what they do might be beneficial for future refactoring or understanding

Sure, can do that. I was trying to stick to the current code style and practices in the file so I left them out. As you might know: there are 2 kind of devs in the market: those who claim comments are bad and those who claim comments are good.

Those who claim they are bad argue that the code should be structured and written in a way that it is clear from reading it what it is doing. 😁 e.g. some comments explaining a workflow, can be replaced by slicing the overlal method into a series of methods/functions which have proper names.

I also rather like comments if they are meaningful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

haha yeah I know but nowadays must developers use some kind of minification process, at least I do with WebPack, and so these comments get dropped from the prod build. If some dev aren't doing that then it's their problem, I would prefer to have more comments but yes if the methods have proper naming and are meaningful then no need to add comments, we are in agreement on that 😉

var data = {};
this.get = function(key) {
return data[key];
}
this.set = function(key, value) {
data[key] = value;
}
this.has = function(key) {
return key in data;
}
this.delete = function(key) {
delete data[key];
}
};

/***
* A sample Model implementation.
Expand All @@ -29,16 +48,19 @@
inlineFilters: false
};


// private
var idProperty = "id"; // property holding a unique row id
var items = []; // data by index
var rows = []; // data by row
var idxById = {}; // indexes by id
var rowsById = null; // rows by id; lazy-calculated
var filter = null; // filter function
var updated = null; // updated item ids
var suspend = false; // suspends the recalculation
var idProperty = "id"; // property holding a unique row id
var items = []; // data by index
var rows = []; // data by row
var idxById = new SlickMap(); // indexes by id
var rowsById = null; // rows by id; lazy-calculated
var filter = null; // filter function
var updated = null; // updated item ids
var suspend = false; // suspends the recalculation
var isBulkSuspend = false; // delays various operations like the
// index update and delete to efficient
// versions at endUpdate
var bulkDeleteIds = new SlickMap();
var sortAsc = true;
var fastSortField;
var sortComparer;
Expand Down Expand Up @@ -91,12 +113,16 @@

options = $.extend(true, {}, defaults, options);


function beginUpdate() {
function beginUpdate(bulkUpdate) {
suspend = true;
isBulkSuspend = bulkUpdate === true;
}

function endUpdate() {
if (isBulkSuspend) {
processBulkDelete();
}
isBulkSuspend = false;
suspend = false;
refresh();
}
Expand Down Expand Up @@ -130,23 +156,49 @@
filterArgs = args;
}

function processBulkDelete() {
var id, item, newIdx = 0;

for (var i = 0, l = items.length; i < l; i++) {
item = items[i];
id = item[idProperty];
if (id === undefined) {
throw new Error("Each data element must implement a unique 'id' property");
}

if(bulkDeleteIds.has(id)) {
idxById.delete(id);
} else {
items[newIdx] = item;
idxById.set(id, newIdx);
++newIdx;
}
}

items.length = newIdx;
bulkDeleteIds = new SlickMap();
}

function updateIdxById(startingIndex) {
if (isBulkSuspend) {
return;
}
startingIndex = startingIndex || 0;
var id;
for (var i = startingIndex, l = items.length; i < l; i++) {
id = items[i][idProperty];
if (id === undefined) {
throw new Error("Each data element must implement a unique 'id' property");
}
idxById[id] = i;
idxById.set(id, i);
}
}

function ensureIdUniqueness() {
var id;
for (var i = 0, l = items.length; i < l; i++) {
id = items[i][idProperty];
if (id === undefined || idxById[id] !== i) {
if (id === undefined || idxById.get(id) !== i) {
throw new Error("Each data element must implement a unique 'id' property");
}
}
Expand All @@ -165,7 +217,7 @@
idProperty = objectIdProperty;
}
items = filteredItems = data;
idxById = {};
idxById = new SlickMap();
updateIdxById();
ensureIdUniqueness();
refresh();
Expand Down Expand Up @@ -205,7 +257,7 @@
if (ascending === false) {
items.reverse();
}
idxById = {};
idxById = new SlickMap();
updateIdxById();
refresh();
}
Expand Down Expand Up @@ -233,7 +285,7 @@
if (ascending === false) {
items.reverse();
}
idxById = {};
idxById = new SlickMap();
updateIdxById();
refresh();
}
Expand Down Expand Up @@ -330,7 +382,7 @@
}

function getIdxById(id) {
return idxById[id];
return idxById.get(id);
}

function ensureRowsByIdCache() {
Expand All @@ -353,7 +405,7 @@
}

function getItemById(id) {
return items[idxById[id]];
return items[idxById.get(id)];
}

function mapItemsToRows(itemArray) {
Expand Down Expand Up @@ -390,9 +442,9 @@
return ids;
}

function updateItem(id, item) {
function updateSingleItem(id, item) {
// see also https://github.com/mleibman/SlickGrid/issues/1082
if (idxById[id] === undefined) {
if (!idxById.has(id)) {
throw new Error("Invalid id");
}

Expand All @@ -404,11 +456,11 @@
if (newId == null) {
throw new Error("Cannot update item to associate with a null id");
}
if (idxById[newId] !== undefined) {
if (idxById.has(newId)) {
throw new Error("Cannot update item to associate with a non-unique id");
}
idxById[newId] = idxById[id];
delete idxById[id];
idxById.set(newId, idxById.get(id));
idxById.delete(id);

// Also update the `updated` hashtable/markercache? Yes, `recalc()` inside `refresh()` needs that one!
if (updated && updated[id]) {
Expand All @@ -419,38 +471,102 @@

id = newId;
}
items[idxById[id]] = item;
items[idxById.get(id)] = item;

// Also update the rows? no need since the `refresh()`, further down, blows away the `rows[]` cache and recalculates it via `recalc()`!

if (!updated) {
updated = {};
}
updated[id] = true;
}

function updateItem(id, item) {
updateSingleItem(id, item);
refresh();
}


function updateItems(id, items) {
for (var i = 0; i < items.length; i++) {
updateSingleItem(id, items);
}
refresh();
}

function insertItem(insertBefore, item) {
items.splice(insertBefore, 0, item);
updateIdxById(insertBefore);
refresh();
}

function insertItems(insertBefore, newItems) {
items.prototype.splice.apply(newItems, [insertBefore, 0].concat(newItems));
updateIdxById(insertBefore);
refresh();
}

function addItem(item) {
items.push(item);
updateIdxById(items.length - 1);
refresh();
}

function addItems(newItems) {
items = items.concat(newItems);
updateIdxById(items.length - newItems.length);
refresh();
}

function deleteItem(id) {
var idx = idxById[id];
var idx = idxById.get(id);
if (idx === undefined) {
throw new Error("Invalid id");
}
delete idxById[id];
items.splice(idx, 1);
updateIdxById(idx);
refresh();
if (isBulkSuspend) {
bulkDeleteIds.set(id, true);
} else {
idxById.delete(id);
items.splice(idx, 1);
updateIdxById(idx);
refresh();
}
}

function deleteItems(ids) {
if (ids.length === 0) {
return;
}

if (isBulkSuspend) {
for (var i = 0, l = ids.length; i < l; i++) {
var idx = idxById.get(id);
if (idx === undefined) {
throw new Error("Invalid id");
}
bulkDeleteIds.set(ids[i], true);
}
} else {
// collect all indexes
var indexesToDelete = [];
for (var i = 0, l = ids.length; i < l; i++) {
var idx = idxById.get(id);
if (idx === undefined) {
throw new Error("Invalid id");
}
idxById.delete(id);
indexesToDelete.push(idx);
}

// Remove from back to front
indexesToDelete.sort();
for (var i = indexesToDelete.length - 1; i >= 0; --i) {
items.splice(indexesToDelete[i], 1);
}

// update lookup from front to back
updateIdxById(indexesToDelete[0]);
refresh();
}
}

function sortedAddItem(item) {
Expand All @@ -461,8 +577,8 @@
}

function sortedUpdateItem(id, item) {
if (idxById[id] === undefined || id !== item[idProperty]) {
throw new Error("Invalid or non-matching id " + idxById[id]);
if (!idxById.has(id) || id !== item[idProperty]) {
throw new Error("Invalid or non-matching id " + idxById.get(id));
}
if (!sortComparer) {
throw new Error("sortedUpdateItem() requires a sort comparer, use sort()");
Expand Down Expand Up @@ -1249,9 +1365,13 @@
"setFilterArgs": setFilterArgs,
"refresh": refresh,
"updateItem": updateItem,
"updateItems": updateItems,
"insertItem": insertItem,
"insertItems": insertItems,
"addItem": addItem,
"addItems": addItems,
"deleteItem": deleteItem,
"deleteItems": deleteItems,
"sortedAddItem": sortedAddItem,
"sortedUpdateItem": sortedUpdateItem,
"syncGridSelection": syncGridSelection,
Expand Down