logical_count: improve memory usage #1341

HashidaTKS · 2022-03-29T06:02:40Z

Overall, I have changed logical_count to immediately unref shards or close temporary objects that are no longer referred like logical_range_filter.

TODO:

Refactor
- Reconsider name of method, class and so on
- Separate common method with logical_range_filter into a common class
  - e.g. detect_time_classify_types
  - If they have not match in common, I will skip this.
More test
- Especially performance test

HashidaTKS · 2022-03-29T06:27:08Z

test/command/suite/sharding/logical_count/log/select.test

+load --table Logs_20150709
+[
+{"timestamp": "2015-02-05 13:49:00"}
+]


I have added this because the expected log #|d| [logical_count][select] <Logs_20150709>: no range index is not written if Logs_20150709 is empty.

Currently, an empty table is skipped before logging that log.
https://github.com/groonga/groonga/pull/1341/files#diff-d3fb57a047ca92c9fc19d900aa1a906a31bc3519eb7c26927516b229705c27a4R290

HashidaTKS · 2022-03-29T06:27:58Z

test/command/suite/sharding/logical_count/log/range_index.test

+load --table Logs_20150709
+[
+{"timestamp": "2015-02-05 13:49:00"}
+]



I have added this because the expected log #|d| [logical_count][range-index] <Logs_20150709>: range index is available is not written if Logs_20150709 is empty.

Currently, an empty table is skipped before logging that log.
https://github.com/groonga/groonga/pull/1341/files#diff-d3fb57a047ca92c9fc19d900aa1a906a31bc3519eb7c26927516b229705c27a4R290

HashidaTKS · 2022-03-29T06:28:30Z

test/command/suite/sharding/logical_count/log/cover_type_all.test

+load --table Logs_20150709
+[
+{"timestamp": "2015-02-05 13:49:00"}
+]


I have added this because the expected log #|d| [logical_count][select] <Logs_20150709>: covered is not written if Logs_20150709 is empty.

Currently, an empty table is skipped before logging that log.
https://github.com/groonga/groonga/pull/1341/files#diff-d3fb57a047ca92c9fc19d900aa1a906a31bc3519eb7c26927516b229705c27a4R290

plugins/sharding/logical_count.rb

test/command/suite/sharding/logical_count/log/cover_type_all.expected

kou · 2022-03-29T08:03:32Z

plugins/sharding/logical_count.rb

-            raise InvalidArgument, message
-          end
+          counter = Counter.new(context)
+          count_result = counter.count


Why did you rename this from total?
It seems that count_result isn't better than total.

I have fixed it.

plugins/sharding/logical_count.rb

This is for #1341.

test/command/suite/sharding/logical_count/log/select.expected

plugins/sharding/logical_count.rb

HashidaTKS · 2022-03-31T03:01:25Z

I am testing performance but ready for review of implementation.
I will write test results to this PR.

kou · 2022-03-31T04:43:35Z

plugins/sharding/logical_count.rb

-          @range_index = range_index
-          @table = shard.table
+      class ShardCountContext < StreamExecuteContext
+        attr_reader :order


It seems that we should be defined in StreamExecuteContext.

How about passing order to StreamExecuteContext#initialize?

Please let me work on this in another PR because it also affects to logical_range_filter.

#1347 is merged

plugins/sharding/logical_count.rb

kou · 2022-03-31T04:52:18Z

plugins/sharding/logical_count.rb

+          ensure_filtered
+
+          if @range_index
+            return count_n_records_in_range
          end


Why is this done before return 0 if @filtered_result_sets.empty??
Can we return 0 here when we have no result sets?

This is related to #1341 (comment).
We don't have any result set when @range_index is defined.

We don't have any result set when @range_index is defined.

In logical_count, @range_index is defined only when we don't need to execute filter and we simply get index_cursor.count (https://github.com/groonga/groonga/pull/1341/files#diff-d3fb57a047ca92c9fc19d900aa1a906a31bc3519eb7c26927516b229705c27a4R191)

kou · 2022-03-31T04:55:10Z

plugins/sharding/logical_count.rb

-            if @post_filter and @dynamic_columns.have_filtered?
-              filtered_table = context.table.select_all
+        def execute_filter(range_index)
+          return if range_index


Umm... "execute_filter does nothing if there is a range index" will confuse developers...
Can we use another approach?

I have fixed it. How about the current approach?

Removed this return if range_index.

Modified to check @range_index before filtering in ShardCountContext.execute.

This is for #1341.

HashidaTKS · 2022-03-31T07:45:07Z

Performance test results.

Condition:

10M record per shard.
Search 30 shards.
Filter with indexed columns

Sorry for my lack of details but I cannot write detailed logs or a schema and so on because it is containing private information...

case: GRN_ENABLE_REFERENCE_COUNT=yes

Execution time

Condition	Execution time
After changed	47 sec
Before changed	54 sec

Memory usage

case: GRN_ENABLE_REFERENCE_COUNT=no
Execution time

Condition	Execution time
After changed	42 sec
Before changed	42 sec

Memory usage

kou · 2022-03-31T08:07:46Z

plugins/sharding/logical_count.rb

+            unless @post_filter.nil?
+              result_set = apply_post_filter(result_set)
+              @temporary_tables << result_set
+            end


Can we move this to StreamShardExecutor?

I have addressed this in another PR.
#1348

#1348 is merged

This is for #1341.

This is for #1341. Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

Fix test data datetime

Modify to return result before filtering when @range_index is not null in execute

- Make order attr sharable - Make postfilter sharable - Avoid needless referring

kou · 2022-04-05T05:59:35Z

plugins/sharding/logical_count.rb

-          end
-          @contexts << ShardCountContext.new(shard, cover_type, range_index)
+      class ShardCountExecutor < StreamShardExecutor
+        include Loggable


This should be in StreamShardExecutor because logger is used in it.

HashidaTKS commented Mar 29, 2022

View reviewed changes

kou reviewed Mar 29, 2022

View reviewed changes

plugins/sharding/logical_count.rb Outdated Show resolved Hide resolved

kou reviewed Mar 29, 2022

View reviewed changes

test/command/suite/sharding/logical_count/log/cover_type_all.expected Outdated Show resolved Hide resolved

HashidaTKS force-pushed the improve_logical_count_memory_usage branch from 20279e0 to 86f788e Compare March 29, 2022 07:18

kou reviewed Mar 29, 2022

View reviewed changes

This was referenced Mar 29, 2022

logical_range_filter: Separate a base of ExecuteContext into another class #1342

Merged

logical_range_filter: Separate a base of ShardExecutor into another class #1343

Merged

kou pushed a commit that referenced this pull request Mar 30, 2022

logical_range_filter: Make ExecuteContext reusable (#1342)

1c3fc7a

This is for #1341.

kou pushed a commit that referenced this pull request Mar 31, 2022

logical_range_filter: make ShardExecutor sharable (#1343)

4779536

This is for #1341.

HashidaTKS mentioned this pull request Mar 31, 2022

Separate Executor to an independent class #1345

Merged

kou pushed a commit that referenced this pull request Mar 31, 2022

logical_range_filer: make StreamExecutor sharable (#1345)

a9b28fd

This is for #1341.

HashidaTKS force-pushed the improve_logical_count_memory_usage branch from fcb3a6a to 0df1ec3 Compare March 31, 2022 02:34

HashidaTKS commented Mar 31, 2022

View reviewed changes

test/command/suite/sharding/logical_count/log/select.expected Outdated Show resolved Hide resolved

HashidaTKS commented Mar 31, 2022

View reviewed changes

plugins/sharding/logical_count.rb Show resolved Hide resolved

HashidaTKS marked this pull request as ready for review March 31, 2022 03:03

HashidaTKS requested a review from kou March 31, 2022 03:03

kou reviewed Mar 31, 2022

View reviewed changes

kou pushed a commit that referenced this pull request Mar 31, 2022

logical_range_filter: Make result_sets enumeration sharable (#1346)

084389f

This is for #1341.

HashidaTKS force-pushed the improve_logical_count_memory_usage branch from cbb42a4 to edb70ab Compare March 31, 2022 06:04

kou reviewed Mar 31, 2022

View reviewed changes

This was referenced Mar 31, 2022

logical_range_filter: Move apply_post_filter to StreamShardExecutor from ShardExecutor #1348

Merged

logical_range_filter: Avoid refering needless index #1349

Merged

kou pushed a commit that referenced this pull request Apr 1, 2022

logical_range_filter: make apply_post_filter sharable (#1348)

af9ba94

This is for #1341.

kou pushed a commit that referenced this pull request Apr 1, 2022

logical_range_filter: make order reusable (#1347)

b31dd34

This is for #1341.

kou added a commit that referenced this pull request Apr 5, 2022

logical_range_filter: Avoid referring needless index (#1349)

7c88cce

This is for #1341. Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

Preparation : Add check open/close logs to tests

9b18081

HashidaTKS added 11 commits April 5, 2022 14:37

Commit current changes

2818ac0

Update expected test results

e73961b

Refactor LogicalCountCommand.run_body

208e913

Regard "@target_table.empty?" as "have_record = false"

4970b3f

Format code

a60c565

Fix test data datetime

Format code

0c7f573

Change to use sharable classes

39c1bdc

Revert a needless change

815f263

Use each_result_set in ShardCountExecutor.execute

8a2779f

Remove range_index null check in execute_filter

ca17b67

Modify to return result before filtering when @range_index is not null in execute

Follow to the master branch

508aff1

- Make order attr sharable - Make postfilter sharable - Avoid needless referring

HashidaTKS force-pushed the improve_logical_count_memory_usage branch from edb70ab to 508aff1 Compare April 5, 2022 05:50

HashidaTKS requested a review from kou April 5, 2022 05:59

kou reviewed Apr 5, 2022

View reviewed changes

kou merged commit 8993ef7 into groonga:master Apr 5, 2022

HashidaTKS mentioned this pull request Apr 13, 2022

doc news: add news topics for 12.0.3 #1355

Merged

github-actions bot mentioned this pull request May 22, 2023

nginx: update bundles version 1.23.4 komainu8/groonga#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

logical_count: improve memory usage #1341

logical_count: improve memory usage #1341

HashidaTKS commented Mar 29, 2022 •

edited

Loading

HashidaTKS Mar 29, 2022

HashidaTKS Mar 29, 2022

HashidaTKS Mar 29, 2022

kou Mar 29, 2022

HashidaTKS Mar 31, 2022

HashidaTKS commented Mar 31, 2022

kou Mar 31, 2022

HashidaTKS Mar 31, 2022

HashidaTKS Mar 31, 2022

HashidaTKS Apr 5, 2022

kou Mar 31, 2022

HashidaTKS Mar 31, 2022

HashidaTKS Mar 31, 2022 •

edited

Loading

kou Mar 31, 2022

HashidaTKS Mar 31, 2022 •

edited

Loading

HashidaTKS commented Mar 31, 2022 •

edited

Loading

kou Mar 31, 2022

HashidaTKS Mar 31, 2022

HashidaTKS Apr 5, 2022

kou Apr 5, 2022

logical_count: improve memory usage #1341

logical_count: improve memory usage #1341

Conversation

HashidaTKS commented Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HashidaTKS commented Mar 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HashidaTKS Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HashidaTKS Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

HashidaTKS commented Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HashidaTKS commented Mar 29, 2022 •

edited

Loading

HashidaTKS Mar 31, 2022 •

edited

Loading

HashidaTKS Mar 31, 2022 •

edited

Loading

HashidaTKS commented Mar 31, 2022 •

edited

Loading