The function actions_synchronize has a parameter $actions_in_code that is really not needed and can be simplified. A look at all the references to actions_synchronize shows that in any case where a $actions_in_code parameter is passed (and not the default array() is used), all that is passed is actions_list(), which we could just do by default in actions_synchronize(). This simplifies actions code and should result in a small gain in performance.

CommentFileSizeAuthor
#10 319404-actions-synchronize-D7.patch2.92 KBdave reid
#6 actions-synchronize-319404-6-D7.patch2.78 KBdave reid
#1 actions-synchronize-319404-1-D7.patch2.72 KBdave reid

Comments

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Active » Needs review
StatusFileSize
new actions-synchronize-319404-1-D7.patch2.72 KB

Attached patch and did some ApacheBench testing on the admin/settings/actions page (which calls actions_synchronize) with 500 requests. Before patch:

Concurrency Level:      1
Time taken for tests:   138.163734 seconds
Complete requests:      500
Failed requests:        0
Write errors:           0
Total transferred:      12327000 bytes
HTML transferred:       12106000 bytes
Requests per second:    3.62 [#/sec] (mean)
Time per request:       276.327 [ms] (mean)
Time per request:       276.327 [ms] (mean, across all concurrent requests)
Transfer rate:          87.13 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   266  275  23.6    270     482
Waiting:      221  242  21.3    238     446
Total:        266  275  23.6    270     482

After patch

Concurrency Level:      1
Time taken for tests:   136.810447 seconds
Complete requests:      500
Failed requests:        0
Write errors:           0
Total transferred:      12327000 bytes
HTML transferred:       12106000 bytes
Requests per second:    3.65 [#/sec] (mean)
Time per request:       273.621 [ms] (mean)
Time per request:       273.621 [ms] (mean, across all concurrent requests)
Transfer rate:          87.99 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   266  273  15.3    271     490
Waiting:      221  241  14.5    239     450
Total:        266  273  15.3    271     490

Result is a very minor increase (no decrease!) in performance and increase in simplicity. Patch also added needed documentation to actions_synchronize for the $delete_orphans parameter.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work

The problem I have is that now actions_list is called twice. And calling actions_synchronize now resets the $actions cache in actions_list. I don't think we want either to happen.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

When we're running actions_synchronize, we would always want to get the latest from actions_list. By running it first in actions_synchronize, any later calls to actions_list are now cached and quick since it was already run.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review

Let's see what others think.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

The main reason for this patch is simplification. Any time the actual $actions_in_code parameter is used, it's just passed array_list() in the function call (i.e. actions_synchronize(actions_list());). It's a pointless parameter that can just be simplified by putting an actions_list() inside the function and remove that parameter.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
StatusFileSize
new actions-synchronize-319404-6-D7.patch2.78 KB

Re-roll after DBTNG update to actions.inc

Status: Needs review » Needs work

The last submitted patch failed testing.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review

Testing error. Resetting status.

Status: Needs review » Needs work

The last submitted patch failed testing.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review
StatusFileSize
new 319404-actions-synchronize-D7.patch2.92 KB

Re-rolling.

Status: Needs review » Needs work

The last submitted patch failed testing.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review

Testing slave #8 failure.

dries’s picture

dries commented

I'm in support for this patch. It simplifies the code indeed.

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community

Interesting patch, let's get it in :)

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.