Comments to 41287 #45

AdrianoDee · 2024-01-18T19:20:58Z

This issue keeps track of the comment addressed for the PRs:

Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287

08 January 2024

Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoVertex/BeamSpotProducer/plugins/alpaka/BeamSpotDeviceProducer.cc

Line 36 in ff839e4

if constexpr (std::is_same_v<Device, alpaka::DevCpu>) {

Is the point here to avoid the memcpy() for CPU backends? If that is the case, I think single instances of this pattern could be acceptable, but for wider use we should develop some abstraction (we have certainly discussed about it with @fwyzard, and I might have started to prototype something some time ago).
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/test/alpaka/VertexFinder_t.dev.cc

Line 1 in ff839e4

#include <cmath>

Does something here verify the results?
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/test/alpaka/VertexFinder_t.dev.cc

Line 109 in ff839e4

class kernel_print {
- Change with:
```
     class Kernel_print {
```
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/test/alpaka/VertexFinder_t.dev.cc

Line 84 in ff839e4

class vertexFinderOneKernel {
- Change with:
```
     class VertexFinderOneKernel {
```
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/vertexFinder.h

Line 24 in ff839e4

class init {
- Change with:
```
     class Init {
```
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/vertexFinder.dev.cc

Line 116 in ff839e4

class vertexFinderKernel2 {
- Change with:
```
     class VertexFinderKernel2 {
```
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/vertexFinder.dev.cc

Line 100 in ff839e4

class vertexFinderOneKernel1 {
- Change with:
```
     class VertexFinderOneKernel1 {
```
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/vertexFinder.dev.cc

Line 74 in ff839e4

class vertexFinderOneKernel { //FIXME: CUDA do not have the split here
- Change with:
```
     class VertexFinderOneKernel {  //FIXME: CUDA do not have the split here
```
Does the FIXME need to be addressed?

Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/vertexFinder.dev.cc

Lines 43 to 45 in ff839e4

    
           // TODO: since nHits is not used anywhere else it gives of an unused variable warning. Check! 
        
           // auto nHits = helper::nHits(tracks_view, idx); 
        
           // ALPAKA_ASSERT_OFFLOAD(nHits >= 3);

Is there something that still needs to be checked, or can this code be removed? If you want to keep the assertion, the nHits declaration could be annotated with [[maybe_unused]].

Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/vertexFinder.dev.cc

Line 30 in ff839e4

class loadTracks {
- Change with:
```
     class LoadTracks {
```
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/splitVertices.h

Line 160 in ff839e4

class splitVerticesKernel {
- Change with:
```
     class SplitVerticesKernel {
```
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/splitVertices.h

Line 45 in ff839e4

#ifndef ALPAKA_ACC_CPU_B_SEQ_T_SEQ_ENABLED

It would be great if this #ifdef could be avoided with e.g. requires_single_thread_per_block_v<TAcc>, or std::is_same_v<alpaka::Dev<TAcc>, alpaka_common::DevHost>, depending which servers to purpose better. I understand this case requires a bit more effort than one if constexpr.
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/sortByPt2.h

Line 74 in ff839e4

sortByPt2(acc, pdata, pws);

Is the delegation useful wrt. just implementing the functionality here?
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/sortByPt2.h

Line 70 in ff839e4

class sortByPt2Kernel {
- Change with:
```
     class SortByPt2Kernel {
```

Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/sortByPt2.h

Lines 59 to 67 in ff839e4

    
           #ifndef ALPAKA_ACC_CPU_B_SEQ_T_SEQ_ENABLED 
        
                 auto& sws = alpaka::declareSharedVar<uint16_t[1024], __COUNTER__>(acc); 
        
                 // sort using only 16 bits 
        
                 cms::alpakatools::radixSort<Acc1D, float, 2>(acc, ptv2, sortInd, sws, nvFinal); 
        
           #else 
        
                 for (uint16_t i = 0; i < nvFinal; ++i) 
        
                   sortInd[i] = i; 
        
                 std::sort(sortInd, sortInd + nvFinal, [&](auto i, auto j) { return ptv2[i] < ptv2[j]; }); 
        
           #endif

Change with:
How about

       if constexpr (not cms::alpakatools::requires_single_thread_per_block_v<TAcc>) {
         auto& sws = alpaka::declareSharedVar<uint16_t[1024], __COUNTER__>(acc);
         // sort using only 16 bits
         cms::alpakatools::radixSort<Acc1D, float, 2>(acc, ptv2, sortInd, sws, nvFinal);
       } else {
         for (uint16_t i = 0; i < nvFinal; ++i)
           sortInd[i] = i;
         std::sort(sortInd, sortInd + nvFinal, [&](auto i, auto j) { return ptv2[i] < ptv2[j]; });
       }

?

Or which code path would be used for a host parallel backend (e.g. TBB)?

Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/fitVertices.h

Line 108 in ff839e4

class fitVerticesKernel {
- Change with:
```
     class FitVerticesKernel {
```
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/fitVertices.h

Line 52 in ff839e4

if (verbose && 0 == threadIdxLocal)
- Change with:
  Could be now
```
       if (verbose && cms::alpakatools::once_per_block(acc))
```
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/clusterTracksIterative.h

Line 219 in ff839e4

if (verbose && 0 == threadIdxLocal)
- Change with:
  Could be now
```
         if (verbose && cms::alpakatools::once_per_block(acc))
```
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/clusterTracksIterative.h

Line 65 in ff839e4

if (verbose && 0 == threadIdxLocal)
- Change with:
  Could be now
```
         if (verbose && cms::alpakatools::once_per_block(acc))
```
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/clusterTracksIterative.h

Line 33 in ff839e4

if (verbose && 0 == threadIdxLocal)
- Change with:
  Could be now
```
         if (verbose && cms::alpakatools::once_per_block(acc))
```
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/clusterTracksIterative.h

Line 20 in ff839e4

class clusterTracksIterative {
- Change with:
```
     class ClusterTracksIterative {
```
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/clusterTracksByDensity.h

Line 228 in ff839e4

class clusterTracksByDensityKernel {
- Change with:
```
     class ClusterTracksByDensityKernel {
```
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/clusterTracksDBSCAN.h

Line 32 in ff839e4

if (verbose && 0 == threadIdxLocal)
- Change with:
  Could be now
```
         if (verbose && cms::alpakatools::once_per_block(acc))
```
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/clusterTracksDBSCAN.h

Line 19 in ff839e4

class clusterTracksDBSCAN {
- Change with:
```
     class ClusterTracksDBSCAN {
```
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/clusterTracksByDensity.h

Line 37 in ff839e4

if (verbose && 0 == threadIdxLocal)
- Change with:
  Could be now
```
       if (verbose && cms::alpakatools::once_per_block(acc))
```
Although given that verbose is constexpr, perhaps
```
       if constexpr (verbose) {
         if (cms::alpakatools::once_per_block(acc))
```
would be better to make sure the conditional code is not emitted when verbose == false.
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/PixelVertexWorkSpaceUtilitiesAlpaka.h

Line 14 in ff839e4

ALPAKA_FN_ACC ALPAKA_FN_INLINE void init(PixelVertexWorkSpaceSoAView &workspace_view) {

I'd suggest to move this function to the same file and namespace where PixelVertexWSSoALayout is defined. I don't think it needs to be inside ALPAKA_ACCELERATOR_NAMESPACE.
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/PixelVertexProducerAlpaka.cc

Line 47 in ff839e4

void produceOnGPU(edm::StreamID streamID, // maybe even remove this and leave only produce?

I think the comment would make much sense.

Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/PixelVertexProducerAlpaka.cc

Lines 76 to 77 in ff839e4

    
           tokenDeviceTrack_ = consumes(conf.getParameter<edm::InputTag>("pixelTrackSrc")); 
        
           tokenDeviceVertex_ = produces();

These could be part of the member initializer list.

Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/PixelVertexProducerAlpaka.cc

Lines 5 to 7 in ff839e4

    
           #include "DataFormats/Common/interface/Handle.h" 
        
           #include "FWCore/Framework/interface/ESHandle.h" 
        
           #include "FWCore/Framework/interface/ConsumesCollector.h"

Suggestion: Not needed

Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/alpaka/PixelVertexProducerAlpaka.cc

Line 10 in ff839e4

#include "HeterogeneousCore/AlpakaCore/interface/module_backend_config.h"
- Suggestion: REMOVE :X:
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/PixelVertexProducerFromSoAAlpaka.cc

Lines 9 to 10 in ff839e4

#include "FWCore/Framework/interface/ConsumesCollector.h"

#include "FWCore/Framework/interface/ESHandle.h"
- Suggestion: Not needed
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/PixelVertexWorkSpaceSoAHostAlpaka.h

Line 10 in ff839e4

class PixelVertexWorkSpaceSoAHost : public PortableHostCollection<PixelVertexWSSoALayout<>> {

How about just using the PortableHostCollection<PixelVertexWSSoALayout<>> directly?
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/PixelVertexWorkSpaceLayout.h

Lines 19 to 20 in ff839e4

namespace workSpace {

using PixelVertexWorkSpaceSoALayout = PixelVertexWSSoALayout<>;

Repeating "Workspace" in the namespace and class names feels redundant.

Also, I think it would be better to define the layout and these type aliases in the same namespace.
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/PixelVertexWorkSpaceLayout.h

Line 1 in ff839e4

#ifndef RecoPixelVertexing_PixelVertexFinding_PixelVertexWorkSpaceLayout_h

Given this file is included in other places than plugins, move it to interface.
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/PixelVertexProducerFromSoAAlpaka.cc

Line 1 in ff839e4

#include "DataFormats/Vertex/interface/ZVertexSoAHost.h"

Can the Alpaka postfix be dropped after the CUDA modules have been removed?
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/PixelVertexProducerFromSoAAlpaka.cc

Line 24 in ff839e4

#undef PIXVERTEX_DEBUG_PRODUCE

Where is this macro defined if it needs to be undefined here?
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/BuildFile.xml

Line 12 in ff839e4

<use name="DataFormats/TrackSoA"/>

This dependence seems specific to RecoPixelVertexingPixelVertexFindingPluginsPortable, so move it there.

On the other hand, dependence to DataFormats/Vertex seems to be missing (if the separate package is kept).
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/BuildFile.xml

Line 11 in ff839e4

<use name="DataFormats/TrackingRecHitSoA"/>
- Suggestion: This doesn't seem to be needed
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/BuildFile.xml

Lines 22 to 23 in ff839e4

<use name="HeterogeneousCore/AlpakaCore"/>

<use name="HeterogeneousCore/AlpakaInterface"/>

Move these two to be specific to RecoPixelVertexingPixelVertexFindingPluginsPortable
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/RecoTracker/PixelVertexFinding/plugins/BuildFile.xml

Lines 14 to 15 in ff839e4

<use name="CUDADataFormats/Vertex"/>

<use name="CUDADataFormats/Track"/>

Move these two to be spcific to RecoPixelVertexingPixelVertexFindingPlugins
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/DataFormats/Vertex/test/alpaka/ZVertexSoA_test.dev.cc

Line 59 in ff839e4

//alpaka::exec<Acc1D>(queue, workDiv, TestVerifyKernel{}, zvertex_view);

Can this line be uncommented?
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/DataFormats/Vertex/test/alpaka/ZVertexSoA_test.dev.cc

Line 21 in ff839e4

for (int32_t j : elements_with_stride(acc, zvertex_view.metadata().size())) {

Is it intentional to fill the entries beyong nvFinal()?

Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/DataFormats/Vertex/src/classes_def.xml

Lines 4 to 14 in ff839e4

    
             <read 
        
               sourceClass="PortableHostCollection<ZVertexSoAHeterogeneousLayout<>>" 
        
               targetClass="PortableHostCollection<ZVertexSoAHeterogeneousLayout<>>" 
        
               version="[1-]" 
        
               source="zVertex::ZVertexSoALayout layout_;" 
        
               target="buffer_" 
        
               embed="false"> 
        
             <![CDATA[ 
        
               PortableHostCollection<ZVertexSoAHeterogeneousLayout<>>::ROOTReadStreamer(newObj, onfile.layout_); 
        
             ]]> 
        
             </read>

How about using the SET_PORTABLEHOSTCOLLECTION_READ_RULES() macro?

Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/DataFormats/Vertex/interface/alpaka/ZVertexSoACollection.h

Line 1 in ff839e4

#ifndef DataFormats_Vertex_interface_ZVertexSoACollection_h

This file defined ZVertexCollection type alias, so ZVertexCollection would seem more consistent file name.
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/DataFormats/Vertex/interface/alpaka/ZVertexUtilities.h

Line 15 in ff839e4

ALPAKA_FN_HOST_ACC ALPAKA_FN_INLINE void init(ZVertexSoAView &vertices) { vertices.nvFinal() = 0; }

I'd suggest to move this function to the file and namespace where ZVertexSoAHeterogeneousLayout<> is defined.

Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/DataFormats/Vertex/interface/alpaka/ZVertexUtilities.h

Lines 11 to 13 in ff839e4

    
           using ZVertexSoALayout = ZVertexSoAHeterogeneousLayout<>; 
        
           using ZVertexSoAView = ZVertexSoAHeterogeneousLayout<>::View; 
        
           using ZVertexSoAConstView = ZVertexSoAHeterogeneousLayout<>::ConstView;

These type aliases are already available in zVertex namespace.

Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/DataFormats/Vertex/interface/alpaka/ZVertexSoACollection.h

Lines 17 to 21 in ff839e4

    
           #ifdef ALPAKA_ACC_CPU_B_SEQ_T_SEQ_ENABLED 
        
             using ZVertexCollection = ZVertexHost; 
        
           #else 
        
             using ZVertexCollection = ZVertexDevice<Device>; 
        
           #endif

How about std::conditional_v? (although if PortableCollection would be used directly, these would not be needed)

Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/DataFormats/Vertex/interface/ZVertexSoADevice.h

Lines 12 to 13 in ff839e4

    
           #include "HeterogeneousCore/AlpakaInterface/interface/config.h" 
        
           #include "HeterogeneousCore/AlpakaInterface/interface/CopyToHost.h"

Suggestion: Not needed here

Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/DataFormats/Vertex/interface/ZVertexLayout.h

Lines 19 to 21 in ff839e4

    
           namespace zVertex { 
        
             // Common types for both Host and Device code 
        
             using ZVertexSoALayout = ZVertexSoAHeterogeneousLayout<>;

The zvertex in both the namespace and the class names feels redundant.

Also, I think it would be better to define the ZVertexSoAHeterogeneousLayout and these type aliases in the same namespace.

Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/DataFormats/Vertex/interface/ZVertexSoADevice.h

Line 16 in ff839e4

class ZVertexSoADevice : public PortableDeviceCollection<ZVertexSoAHeterogeneousLayout<>, TDev> {

I'd suggest to move to use the PortableDeviceCollection<zVertex::ZVertexSoALayout> directly. Or is the size being in the type somehow important?

Same for the host collection.
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/DataFormats/Vertex/interface/ZVertexLayout.h

Line 7 in ff839e4

GENERATE_SOA_LAYOUT(ZVertexSoAHeterogeneousLayout,

Is the Heterogeneous needed in the name?

Should the layout be defined in a namespace, e.g. reco?
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/DataFormats/Vertex/interface/ZVertexLayout.h

Line 4 in ff839e4

#include <Eigen/Core>
- Suggestion: Doesn't seem to be used
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/DataFormats/Vertex/README.md

Line 1 in ff839e4

# Vertex CUDA Data Formats

All CUDA-isms should in the README should be converted to Alpaka.
Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287 (comment) by @makortel

cmssw/DataFormats/Vertex/BuildFile.xml

Line 1 in ff839e4

<use name="alpaka"/>

Also here I'd ask if a new package is really needed, or if the existing DataFormats/VertexReco could be used.

If a separate package is kept, how about VertexSoA to be more consistent with the other (possibly) new packages with *SoA name?

The text was updated successfully, but these errors were encountered:

AdrianoDee mentioned this issue Jan 25, 2024

Pixel Alpaka Migration: Vertexing [V] cms-sw/cmssw#41287

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments to 41287 #45

Comments to 41287 #45

AdrianoDee commented Jan 18, 2024 •

edited

Loading

Comments to 41287 #45

Comments to 41287 #45

Comments

AdrianoDee commented Jan 18, 2024 • edited Loading

08 January 2024

AdrianoDee commented Jan 18, 2024 •

edited

Loading